Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract fetchYaml and maybe other fetch functions #2518

Closed
paulmelnikow opened this issue Dec 12, 2018 · 4 comments
Closed

Extract fetchYaml and maybe other fetch functions #2518

paulmelnikow opened this issue Dec 12, 2018 · 4 comments
Labels
core Server, BaseService, GitHub auth

Comments

@paulmelnikow
Copy link
Member

See discussions:

@paulmelnikow paulmelnikow added the core Server, BaseService, GitHub auth label Dec 12, 2018
@paulmelnikow
Copy link
Member Author

Should we keep this open?

The thing we could still do is delegate from _requestJson, _requestYaml, etc. to functions which can be invoked on any instance of BaseService.

@chris48s
Copy link
Member

Aah yes. I was unsure on this. My initial thought was we could extract them to stand-alone functions. The reason I didn't do this was because inside the _requestFormat() functions we're calling this._request() and this.constructor._validate() so _requestFormat() doesn't extract so nicely outside of the class.

We could do it so that we pass in a requester function and a validator function as params, or so that we can pass an instance of BaseService as a param. Another option might be to try and do it as a mixin. I'm not really sure which of those makes the most/least sense.

Given the F-Droid badge could just extend BaseYamlService I decided to just leave it and deal with it once we've got an example where we actually need this abstraction.

@paulmelnikow
Copy link
Member Author

pass an instance of BaseService as a param

That's the approach I've taken before:

async function fetch(serviceInstance, { url, qs = {}, errorMessages }) {

@paulmelnikow
Copy link
Member Author

Gonna close this for now, as it is not seeming pressing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth
Projects
None yet
Development

No branches or pull requests

2 participants