Skip to content
This repository was archived by the owner on Jun 17, 2019. It is now read-only.

Allow provider count option#34

Open
sean-hill wants to merge 1 commit intoholidayextras:masterfrom
sean-hill:master
Open

Allow provider count option#34
sean-hill wants to merge 1 commit intoholidayextras:masterfrom
sean-hill:master

Conversation

@sean-hill
Copy link
Copy Markdown

No description provided.

@hpoom
Copy link
Copy Markdown
Contributor

hpoom commented Jun 21, 2016

I don't think this is the best solution. The issue I see is that 1 gets passed in to select provider 1 but on line 90 it increments providers when it fails, but this would not work if provider was 1 and we would loose this backup. Also there is no protection or range checking to stop somebody passing in -1 or 99 as a value.

PR 33 is about to change this as well to make calls to all providers happen and fastest one win. I suggest best route forward would be to wait for #33 to be merged then we can look at a solution to turn on and off providers.

I still feel passing in a config would be best. So that you can override all of providers.js and add in new custom providers not even covered by this NPM module.

@hpoom
Copy link
Copy Markdown
Contributor

hpoom commented Jun 21, 2016

@jamescogley have I overlooked anything here? What are your thoughts on the best way to achieve what @sean-hill needs?

@sean-hill
Copy link
Copy Markdown
Author

You guys are the best :)

@jamescogley
Copy link
Copy Markdown
Contributor

@hpoom I agree, passing the providers in as config would be the best option, it would give the most flexibility

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants