-
Notifications
You must be signed in to change notification settings - Fork 21
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
Refactoring settings #294
Refactoring settings #294
Conversation
This looks like a lot of changes. Can you explain what's going on at the high level? |
It looks like Boldi is making my janky implementation less spaghetti-like, by using named dictionary types, and spreading out some of the data from the definition of the Settings class. This is probably a good idea. |
I have updated the description of the PR, intending to explain what's going on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I have suggested a few changes related to dynamically setting attributes which makes the code difficult to read and maintain
Okay, I have modified the implementation to use built-in methods instead of tricks with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, shall we merge it then?
Most information related to settings has been stored in
common.py
, and the implementation ofsettings_dialog.py
was not the cleanest.This PR intends to:
settings.py
file, andMost changes are simply moving data to settings.py and extracting methods in settings_dialoge.py (which are sometimes further abstracted to reduce code repetition). Furthermore, some
TypedDict
s are defined to standardise the data insettings.py
.