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.

Sign up for GitHub

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

Jump to bottom

ENUM refactor #216

Open
wants to merge 11 commits into
base: staging
Choose a base branch
from
Open

ENUM refactor #216

wants to merge 11 commits into from

Conversation

Flavsditz
Copy link

This is done to tackle the ENUM refactoring as per this discussion.

I have gone through the code and found several entities that could be transformed into ENUMs and done so.
I have created a folder for the ENUMs so it is easy to find and maintain them. There are two, one in the motorlib and one in the uilib since the latter contains ENUMs that are only used there.

There are a lot of files changed, I apologize, but there were mentions throughout the code. Now at least a typo wouldn't introduce a problem.

I would like to discuss the "Unit Enums" I have created: Initially, I thought it would be a good idea to keep them separate into their own areas but that led to some files containing 1 or 2 units only. While this is not bad (it is not like we are paying by the file here), one could also argue to just have 1 single ENUM called Unit and have them all there.

I would like to hear your opinion and I am happy to refactor otherwise.

@Flavsditz
Copy link
Author

I seem to notice that these changes are not backward compatible with the old preferences file... It would throw an error if you try to update it...
I am looking if there is a way to offer backwards compatibility. This doesn't block a discussion but probably the merge unless we are ok of saying people need to erase their old preferences and propellants files

@benrussell11
Copy link

benrussell11 commented Jun 12, 2023 via email

@Flavsditz
Copy link
Author

@benrussell11 yes I thought so and I have a couple of ideas how to deal with this that I am studying...
Would you be able to provide some files so I can have some real-life comparisons?

@benrussell11
Copy link

benrussell11 commented Jun 12, 2023 via email

@reilleya
Copy link
Owner

Thanks for this substantial PR! I will absolutely find time to look it over this week. In the meantime, I think it should be possible to write a migration in uilib/fileIO.py for the preference files if all that changed was the specific strings used as keys/values. Here are my preferences and propellants (renamed from .yaml for uploading) as a reference, let me know if you have any questions on how to write the migration.

preferences.txt
propellants.txt

@Flavsditz
Copy link
Author

You bet … let me know when you need my preference file and propellant files.

Hey @benrussell11 feel free to send me right away :) Having two real-world examples is definitely helpful

@benrussell11
Copy link

benrussell11 commented Jun 13, 2023 via email

@benrussell11
Copy link

You bet … let me know when you need my preference file and propellant files.

Hey @benrussell11 feel free to send me right away :) Having two real-world examples is definitely helpful

Here you go and thanks for helping out.
preferences.txt
requirements.txt

@Flavsditz
Copy link
Author

Thanks for the files both of you! (@benrussell11 you sent a requirements.txt file instead of the propellant file though ^^ but it doesn't matter... I think I had enough to pinpoint the issue)
Well I added a fix.

This was happening upon calling the pyYAML constructor so I couldn't solve it by checking the version number of the file. (btw nice solution there!). The problem in itself was just a single ENUM (fileType) where it was trying to find !!python/object/apply:uilib.fileIO.fileTypes which doesn't exist anymore and I simply did a search and replace by replacing with !!python/object/apply:uilib.enums.fileType.FileType.

Now this works. It might look a bit hacky and, in hindsight, I could have gone the route of using feature flags properly. I can still do that but it would take more time to check what was changed and also add somewhat a good number of extra code for an undefined amount of time (we can never guarantee everyone is migrated). So I like to consider my solution as a "data migration" if you will ^^

Btw, I noticed that the preferences file contains the version, but it won't update until the user actually open the preferences window... so in theory a user which is very satisfied with their preferences will never open that file and might remain indefinitely on an older version of the file. Might it be worth to save it on exit?

@reilleya reilleya self-requested a review June 19, 2023 21:12
reilleya
reilleya requested changes Jun 19, 2023
View reviewed changes
Copy link
Owner

@reilleya reilleya left a comment

Choose a reason for hiding this comment

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

Thank you so much for taking on this effort! Looks great for the most part, just a couple of minor suggestions.

motorlib/enums/units/densityUnit.py Outdated Show resolved Hide resolved
motorlib/grain.py Outdated Show resolved Hide resolved
motorlib/enums/inhibitedEnds.py

# Python 3.11 supports `StrEnum` that would make this a bit more concise to write
# https://docs.python.org/3/library/enum.html#enum.StrEnum
class InhibitedEnds(str, Enum):
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should try to take this opportunity to make a migration that converts "top"->"forward" and "bottom"->"aft".

Copy link
Author

Choose a reason for hiding this comment

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

As far as I can see this would only be present on the .ric files. Is this assumption correct?
So that the necessary migration can be done I don't want to miss anything.

Any chance for a more complex .ric file to be made available so I can use it for testing too?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the delay! Here's a kind of silly example that uses all 4 inhibitor configurations:
example.ric.txt
and what it looks like on my end, so you can verify nothing changed:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reilleya reilleya reilleya requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Flavsditz @benrussell11 @reilleya