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

ClassManager Rewrite #101

Merged
merged 3 commits into from
Mar 2, 2024
Merged

Conversation

infeeeee
Copy link
Collaborator

@infeeeee infeeeee commented Mar 2, 2024

  • Part 2 of App settings again #90 separation
  • Rewrite ClassManager:
    • Use pathlib. It makes a lot of file and directory commands much simpler, nicer and more readable.
    • ClassManager functionality didn't change, merged functions which were called only once, removed unused code.
  • New method: MyApp.App.getRootPath(): Returns the absolute filepath of the installation. It can be useful at other situations in the future.

Question to @richibrics:

The 2 subclasses of ClassManager became really short. Maybe they could be replaced by something like this:
EntityClassManager() = ClassManager(KEY_ENTITIES)
WarehouseClassManager() = ClassManager(KEY_WAREHOUSES)

Because these subclasses are differ only by the base path, and I don't really see how they can change in the future.

What do you think of this?

@infeeeee infeeeee mentioned this pull request Mar 2, 2024
5 tasks
@richibrics
Copy link
Owner

What would be the key ? The path ? If so, I'd pass a list of path so that it's ready for future implementations of custom entities

@infeeeee
Copy link
Collaborator Author

infeeeee commented Mar 2, 2024

I would pair the path to the key in consts.py, so we don't have to write the path everywhere ClassManagers are used

@richibrics
Copy link
Owner

Yes, we could use it with a fixed key. What do you think in pairing the key with a list of paths (for future custom entities) ?

@infeeeee
Copy link
Collaborator Author

infeeeee commented Mar 2, 2024

I just pushed a new commit, I had to change some parts in Configurator as well for this to work.

It will make the whole Configurator workflow much easier, by having the same class a lot of duplicate methods in Configurator can be merged, but that's for the next PR.

Copy link
Owner

@richibrics richibrics left a comment

Choose a reason for hiding this comment

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

Great I approve it

@infeeeee infeeeee merged commit 249d879 into richibrics:main Mar 2, 2024
1 check passed
@infeeeee infeeeee deleted the ClassManager-rewrite branch March 2, 2024 22:13
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.

2 participants