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

App settings again #90

Closed
wants to merge 26 commits into from
Closed

App settings again #90

wants to merge 26 commits into from

Conversation

infeeeee
Copy link
Collaborator

@infeeeee infeeeee commented Dec 30, 2023

This PR monster will be separated to smaller separate requests:

Changes:

  • New items in Configurator Menu: Log Settings and App Settings under a common menu item Settings.
  • On App Settings you can change the update interval of all entities. Default is 10 seconds as before.
  • Log Settings: Change log level separately for console and file logging, enable/disable file logging (disabled by default), and change the folder of file logs. Default paths are form Implement Configuration and Log dirs classes #61
  • Change existing configuration of any entities and warehouses. (This is actually a side product of the new settings, after I wrote that code, I figured out it could be used here as well)
  • Configuration is now a separate class, not just a dict
  • TerminalDetection class for terminal related methods.

Changes for existing users:

  • The default file log path is changed, and file logging is disabled by default.

Secondary Update interval is commented out yet. I figured out that you cannot change the update interval on sensor level, only on entity level, and I think it would make more sense if it could be changed on a sensor level. That way an entity could have one frequent sensor and one infrequent. What do you think?

Retry #84

TODO:
- [x] Logging starts with a default log level before loading the configuration
- [x] After config loaded log level changes to the one in setting
- [x] Add setting to enable/disable file logging
- [x] Add setting for path to logdir
- [x] Logging should be saved to some cache at the beginning. If file logging is enabled, save the cache to file, so nothing is lost. If file logging is not enabled drop the cache.
- [ ] Implement secondary update interval in some sensors.

@infeeeee infeeeee mentioned this pull request Dec 30, 2023
@infeeeee infeeeee added the enhancement New feature or request label Jan 2, 2024
@infeeeee infeeeee marked this pull request as ready for review January 14, 2024 21:09
@infeeeee infeeeee requested a review from richibrics January 14, 2024 21:09
@infeeeee
Copy link
Collaborator Author

@richibrics Take a look, I think it's working now. I copied the code for the default Logs folder paths from #61

I will update the top posts with listing what actually changed. E.g. you can edit settings in the UI now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is some code in Configurator where it calculates the the height of the terminal, it could be moved here. Also the width of the terminal could be calculated here for Logging

Copy link
Owner

Choose a reason for hiding this comment

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

Yes it could be moved there since it's a terminal related class and we would free the Configurator of extra things

@richibrics
Copy link
Owner

Hi @infeeeee , sorry for the late reply.
I though I wanted to check if what I did in the Path configuration classes was correct but I have never found the time for it unfortunately.
Do you think we could integrate the classes I was preparing or keep them away for now ?

@infeeeee
Copy link
Collaborator Author

Keep them away for now.

I'm not sure they are needed at all, simply using pathlib solves most of the problems, and I use it everywhere where I have to work with paths. But let's see at the end.

@infeeeee infeeeee marked this pull request as draft February 12, 2024 17:27
@infeeeee infeeeee marked this pull request as ready for review February 24, 2024 01:05
@infeeeee
Copy link
Collaborator Author

Rewrote ClassManager.

Settings work similarly to Warehouses and Entities. Settings are loaded to the SettingsManager singleton on startup, entities can ask configuration from there. Log settings loaded separately to Logger.

@richibrics Take look, I consider this ready to be pulled now.



class AppSettings(ConfiguratorObject):
"""Singleton for storing AppSettings, not related to Entites or Warehouses """
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not singleton, should update docstring

@infeeeee
Copy link
Collaborator Author

@richibrics This PR grew huge. Should I separate it to smaller ones, so it's easier to review? I can create 2 or maybe 3 smaller PRs instead of this beast. What do you think?

@richibrics
Copy link
Owner

Well if it's splittable it would be better for the understanding, but is it doable ?

@infeeeee
Copy link
Collaborator Author

Yes, I think I can separate to these parts:

  • Small fixes on entities
  • Editing existing configuration
  • ClassManager Rewrite
  • Logger rewrite and LogSettings
  • AppSettings

I won't close this so I can track changes in the other PRs.

@infeeeee infeeeee marked this pull request as draft February 26, 2024 19:08
@infeeeee infeeeee mentioned this pull request May 4, 2024
@infeeeee infeeeee closed this May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants