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

Settings Refinement (#66) #67

Merged
merged 2 commits into from
Aug 21, 2024

Conversation

Iain-Crowe
Copy link
Contributor

@Iain-Crowe Iain-Crowe commented Jul 25, 2024

Pull Request: Refactor Settings Management (#66)

Description

This pull request introduces significant refactoring to the settings management system. The key changes are as follows:

  1. Code Refactoring:
  • Merged redundant imports
  • Moved hardcoded paths to use os.path.join for better portability
  • Introduced global SETTINGS dictionary to hold settings in memory.
  • Added type hints for readability.
  • Encapsulated private methods and constants for better code organization.
  1. Error Handling:
  • Created SettingsNotFoundError to handle cases where settings are not initialized properly.
  • Improved error handling during file operations, including JSON-specific errors.
  1. Functionality Enhancements:
  • Added init_settings() method to initialize settings from JSON file.
  • Added _dump_settings() method to save the current state of SETTINGS to the JSON file.
  • Modified/renamed update_setting() and read_setting() methods to interact with global SETTINGS dictionary.
  • NOTE: init_settings() should only be called once, please ask for details on best usage, if unsure.
  1. Licensing and Documentation:
  • Added the GPL license to top of file.
  • Included docstrings for all methods to explain purpose and usage.
  • Added a note about how we should handle settings within the project.

Issues Addressed

How to Test:

  1. Run the script to ensure the settings are being properly loaded and saved.
  2. Modify a setting and verify the changes are being reflected in the JSON file.
  3. Ensure appropriate error messages are displayed when the settings file is missing or corrupted.

Code Sample

if __name__ == "__main__":
    init_settings()
    
    print(read_settings("showtc")
    update_setting("showtc", False)
    print(read_settings("showtc")

Additional Notes

Future improvements could include moving the custom class into a dedicated exceptions file and integrating a logging library for better error tracking (noted in comments).

  • Fixed requirements.txt, should be able to simply run pip install -r requirements.txt
  • Updated settings usage in other files front.py and orca.py
  • Renamed setings to settings

Copy link
Member

@BlazarKnight BlazarKnight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks so much i am so sorry for the delay in review.

@BlazarKnight BlazarKnight merged commit e461262 into true-public-access:main Aug 21, 2024
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.

None yet

2 participants