Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactoring settings #294

Merged
merged 9 commits into from
Jul 9, 2024
Merged

Refactoring settings #294

merged 9 commits into from
Jul 9, 2024

Conversation

boldar99
Copy link
Collaborator

@boldar99 boldar99 commented Jul 7, 2024

Most information related to settings has been stored in common.py, and the implementation of settings_dialog.py was not the cleanest.

This PR intends to:

  1. make the settings related code type-safe,
  2. move the information related to settings to a new settings.py file, and
  3. clean up the implementations related to settings.

Most 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 TypedDicts are defined to standardise the data in settings.py.

@boldar99 boldar99 added the Draft label Jul 7, 2024
@boldar99 boldar99 changed the title WIP: Refactoring settings Refactoring settings Jul 8, 2024
@boldar99 boldar99 removed the Draft label Jul 8, 2024
@RazinShaikh
Copy link
Collaborator

This looks like a lot of changes. Can you explain what's going on at the high level?

@jvdwetering
Copy link
Collaborator

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.

@boldar99
Copy link
Collaborator Author

boldar99 commented Jul 8, 2024

I have updated the description of the PR, intending to explain what's going on.

Copy link
Collaborator

@RazinShaikh RazinShaikh left a 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

@boldar99
Copy link
Collaborator Author

boldar99 commented Jul 8, 2024

Okay, I have modified the implementation to use built-in methods instead of tricks with setattr.

@boldar99 boldar99 requested a review from RazinShaikh July 8, 2024 21:15
Copy link
Collaborator

@RazinShaikh RazinShaikh left a 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?

@boldar99 boldar99 merged commit a65faf7 into master Jul 9, 2024
2 checks passed
@boldar99 boldar99 deleted the settings branch July 9, 2024 09:02
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants