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

[PROPOSAL] Better, simpler and unified config #248

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

Kyrela
Copy link
Collaborator

@Kyrela Kyrela commented Dec 17, 2024

Warning

The purpose of this PR is to discuss a potentially better way to handle configuration. While it is 100% functional as it is, it involves huge changes to the script, especially the user interactions, and may introduce unwanted behavior, and should be discussed before merging anything.

This takes a different approach to configuration, with two goals in mind: simplicity, both for users and maintainers, and dynamism.

Replaces the old static configuration system with a dynamic Config class that supports nested merges, defaults, YAML serialization, and direct access as attributes. Introduces improved CLI options for better usability and removes deprecated configuration files and structures.

Benefits

For developers

  • A single way to access all user-configurable options, flags and arguments, regardless of their origin, through the CONFIG variable.
  • No need to pass command line arguments through each method where they are needed.
  • Much less verbose and fail-safe way to access options:
    • Using CONFIG[]: CONFIG["apprise.notify.login-code"] instead of CONFIG["apprise"]["notify"]["login-code"].
    • Using CONFIG.get(): CONFIG.get("apprise.notify.login-code") instead of CONFIG.get("apprise").get("notify").get("login-code") (Note that the old method would throw an error if CONFIG.get("apprise") returns None, because the .get method is not a valid method for NoneType, but the new one allows you to get None even if the apprise key is not in the config.
    • Using direct attribute access: CONFIG.activities.ignore (equivalent to CONFIG["activities"]["ignore"]).
    • All previous access options are still perfectly usable; the new ones are just shorter and allow better readability.
  • Easier to test, thanks to the --debug, --email, --password and --config flags, and the 0-configuration-run.

For users

  • A single documented configuration file, instead of multiple files created in different ways.
  • No useless command-line flags.
  • More useful command line flags (especially the --email and --password flags, which allow 0-configuration-run).
  • More advanced configuration options through the configuration file, such as activities.search (useful to quickly add your own language searches), cooldown.min/cooldown.max (useful to speed up the program or try to avoid banning it at most), previously command-line specific options, and more. For the full list of allowed configuration variables, see the README.md.
  • Allows partial configuration (thanks to the new Config class), no need to rewrite/copy the whole configuration, just add what you need.

Changes

Added

  • The Config type, which extends the functionality of dictionaries to allow simpler ways to manage configuration.
  • Ability to configure the global proxy from the configuration file.
  • Ability to configure headless mode from the configuration file.
  • Ability to configure the activity titles to be searched from the configuration file.
  • Ability to configure the minimum and maximum cooldown between retirements from the configuration file.
  • Ability to configure the type of search to be performed from the configuration file.
  • The --config flag, which can be used to specify the configuration path.
  • The --create-config flag, which can be used to create an empty configuration file (instead of copying and renaming a template file). Note that the created file will already be populated with any configuration options passed from the command line.
  • The --email and --password flags, which allow the script to be run for a single user, even without a configuration file.
  • The --debug flag, which allows to set the logging level to DEBUG.

Changed

  • All configuration files have been merged into a single file, config.yaml.
  • All command line arguments are merged into the configuration (CONFIG) and accessed through it.
  • If no account is specified (either from the command line or from the configuration file), a message is displayed to help the user create a configuration file or specify an account from the command line.
  • The account.username values have been renamed to account.email, both internally and in the configuration.
  • Some configuration variables have been moved, such as apprise.notify.uncaught-exception.enabled, which has been moved to apprise.notify.uncaught-exception (because the enabled key is redundant), and apprise.notify.incomplete-activity.ignore, which has been moved to activities.ignore (because it does not only affect apprise). For the full list of allowed configuration variables, see the README.md.
  • Rewrote README.md, .github/ISSUE_TEMPLATE/bug_report.yml, test/test_main.py and test/test_utils.py to reflect these changes.

Removed

  • The Account data model.
  • The --chromeversion flag, deprecated as of 982a592.
  • The --verbosenotifs flag, deprecated as of 77fc8f0.
  • The default configuration files and templates, now unused.

Replaces the old static configuration system with a dynamic `Config` class supporting nested structures, defaults, YAML serialization and direct access as attributes. Introduces enhanced CLI options for better usability and removes deprecated configuration files and structures.
Updated the configuration documentation to clarify that several settings can now be overridden through command-line arguments. This improves flexibility and provides clearer guidance for users to customize behavior.
@klept0
Copy link
Owner

klept0 commented Jan 9, 2025

Im testing your repo on a separate account and it seems to be working relatively well. I will try and reach out to Cal and see how he would like to proceed as he was working on it and I do not want to change any files he may have pending.

@Kyrela
Copy link
Collaborator Author

Kyrela commented Jan 20, 2025

Hello, got any news?

@klept0
Copy link
Owner

klept0 commented Jan 21, 2025

Was working well for me - I was hoping @cal4 would be able to check it if possible to see if I missed anything. If he doesn't get to look at it by the time I get back this evening I will merge things accordingly.

@Rippenkneifer
Copy link

i like the —email and —password flag(?arg)'s , this can be very useful specially for newbies, also it is interesting what will happen if it is used 2 times in the same folder with different Accounts.

Also Adding a cooldown into config.yaml is useful.
And Changing accounts.json into config.yaml can be good.
Using CONFIG.get() avoids errors for nonexistent keys (e.g. NoneType attributes).

maybe adding one thing to the --create-config Flag(?arg) could be that he checks if already one exists before creating one file and adding some messages into that, so if someone does this mistakenly he will not override his file, and instead getting a warning Message about the already existing File, but in all it sounds very useful.

@Kyrela
Copy link
Collaborator Author

Kyrela commented Jan 21, 2025

maybe adding one thing to the --create-config Flag(?arg) could be that he checks if already one exists before creating one file and adding some messages into that, so if someone does this mistakenly he will not override his file, and instead getting a warning Message about the already existing File

It already does!

py .\main.py -C
ERROR:root:[CONFIG] A file already exists at 'C:\Users\username\Documents\Programmation\MS-Rewards-Farmer\config.yaml'

@klept0 klept0 merged commit 770e318 into klept0:develop Jan 21, 2025
1 check passed
@Kyrela Kyrela deleted the unified-config branch January 22, 2025 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants