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

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?

zxlive/settings.py Outdated Show resolved Hide resolved
@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
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.

3 participants