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

[Feature] Added a better config merging system #1874

Merged
merged 14 commits into from
Aug 1, 2016
Merged

[Feature] Added a better config merging system #1874

merged 14 commits into from
Aug 1, 2016

Conversation

Andrerm124
Copy link
Contributor

Fixed conflicts and reimplemented:
Ignore all pokemon not on Sniper Filter
Time for pokestop arrival

Fixed conflicts and reimplemented:
Ignore all pokemon not on Sniper Filter
Time for pokestop arrival
So I decided to go through every single setting and supply a default
value to each one. If everyone uses this as standard in the future there
should be no need to switch out configs, it will simply update the old
config with the new default values.

IF THIS WORKS, MORE WORK WILL BE NECESSARY TO REMOVE OLD CONFIG
REPLACEMENT.
@Andrerm124
Copy link
Contributor Author

I'd really like to have a second opinion on whether my method should be the new standard and whether I should rip out the old implementation.

@recon88 recon88 added the ready label Aug 1, 2016
@dddbliss
Copy link
Contributor

dddbliss commented Aug 1, 2016

If you look in Settings.cs in Logic, around line 482 you will see where we apply a bunch of defaults, if you can verify we no longer need these and can use DefaultValue attributes insteads, that would be awesome.

@recon88 recon88 changed the title Conflict Fixes Conflict Fixes (fixes #1673) Aug 1, 2016
@recon88 recon88 changed the title Conflict Fixes (fixes #1673) Conflict Fixes ( fixes #1673 ) Aug 1, 2016
@Andrerm124
Copy link
Contributor Author

Andrerm124 commented Aug 1, 2016

@dddbliss I did what you suggested and it seems to work fine.

I think this method may overwrite the need for a lot of the VersionCheckState code (Specifically TransferConfig())

@Andrerm124
Copy link
Contributor Author

Andrerm124 commented Aug 1, 2016

Do not commit, found an issue with fresh installs. Trying to resolve issue.

  • Issue Resolved

Previously the default values were not being pushed to a GlobalSettings
object and thus would not be used.
@Andrerm124 Andrerm124 changed the title Conflict Fixes ( fixes #1673 ) [Feature] Added a better config merging system Aug 1, 2016
@BornSupercharged
Copy link
Contributor

Let us know when the issue has been resolved.

@Andrerm124
Copy link
Contributor Author

Already fixed, it's ready for a commit I believe. Unless you can see any issues that is.

@Andrerm124
Copy link
Contributor Author

@SimplyPHP Could you please commit this request? I'm heading off for the night and won't be able to fix any conflicts that pop up, thanks :)

@BornSupercharged BornSupercharged merged commit b66c98a into NecronomiconCoding:master Aug 1, 2016
schnapster pushed a commit to schnapster/NecroBot that referenced this pull request Dec 8, 2016
[Feature] Added a better config merging system
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.

4 participants