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

Multiple Config File Support #771

Merged
merged 5 commits into from
Jul 25, 2016
Merged

Multiple Config File Support #771

merged 5 commits into from
Jul 25, 2016

Conversation

ColeGreenlee
Copy link
Contributor

Adds ability to drop config files into the 'configs' dir whilst developing and have them ignored by git

Adds ability to drop config files into the 'configs' dir whilst
developing and have them ignored by git
@ghost
Copy link

ghost commented Jul 25, 2016

Does it support run bot with multiple configs as once?

@ColeGreenlee
Copy link
Contributor Author

The bot can currently use any .json file to run, all this PR will do is move all the configs to a 'configs' directory and have your commits ignore them, while still not ignoring the example configs.

@fredrik-hellmangroup
Copy link
Contributor

we are going to make changes to config.json and release_config.json. once those are updated, please review this PR again and make the updates necessary for it to work.

@ColeGreenlee
Copy link
Contributor Author

Even with changes, this PR shouldn't affect anything but help developers test on different configs. I can make changes accordingly once we merge the config files if it does affect anything

#Multiple config
configs/
!configs/config.json.example
!configs/release_config.json.example
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the config examples be added to the commits ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what this does. Ignore the config directory, but allow the two example files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^Yep!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you dont comit in the config and example files, you just delete them from directory with this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly ^ :p

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooops! I thought I had moved those two to the configs folder for his PR. If I did that could we move forward with this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think once you have both your files in the configs folder I don't see why we couldn't merge

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ColeGreenlee adding a warning for people so they move their config.json files to the new directory would be pretty good as well.

Like trying to open it on configs directory first and if not there adding a Warning print "config.json not found, please add one to the configs directory"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added warnings in latest commit. Thanks for the suggestion!

@ColeGreenlee
Copy link
Contributor Author

Okay, example files have been re-included. Was just a missing * form the .gitignore 👍

@ColeGreenlee
Copy link
Contributor Author

Added the warnings as suggested. Is this PR good to go?

@dmateusp
Copy link
Contributor

waiting for a second review but it should be good to go !

@eggins
Copy link
Contributor

eggins commented Jul 25, 2016

+1

@dmateusp dmateusp merged commit c13a400 into PokemonGoF:dev Jul 25, 2016
@ColeGreenlee ColeGreenlee deleted the dev branch July 25, 2016 09:53
@ColeGreenlee ColeGreenlee restored the dev branch July 25, 2016 09:53
@ColeGreenlee ColeGreenlee deleted the dev branch July 25, 2016 09:53
@ColeGreenlee
Copy link
Contributor Author

Thanks!

@douglascamata
Copy link
Member

@ColeGreenlee can I ask for another PR with information about new folder for config files in README?

@ColeGreenlee
Copy link
Contributor Author

Yeah, I'd be more than happy to document this.

@TSM-EVO
Copy link

TSM-EVO commented Jul 25, 2016

Please do document this. What are the arguments for running configs from this /configs/ folder

MFizz pushed a commit to MFizz/PokemonGo-Bot that referenced this pull request Jul 29, 2016
* Multiple Config File Support

Adds ability to drop config files into the 'configs' dir whilst
developing and have them ignored by git

* Modified .gitignore

* Fixed Example files not being included

* Added warnings to PokeCLI based on config availability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants