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

NPI-3458 implement solution types utility classes #48

Merged
merged 6 commits into from
Sep 3, 2024

Conversation

treefern
Copy link
Collaborator

This PR adds utility classes to more formally capture the definitions of solution types defined by IGS. This avoids identifying solution types by short strings throughout the codebase, and allows additional details of each solution type to potentially be captured here as well.

@treefern treefern self-assigned this Aug 23, 2024
Copy link
Collaborator

@ronaldmaj ronaldmaj left a comment

Choose a reason for hiding this comment

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

This looks good to me, adds new code but doesn't alter anything existing so won't cause issues. Plus this will help heaps in our Ops space in making things clearer.

The one thing that could be added are unit tests on these methods, but if this needs to be in the repo to help with testing on Ops, happy for this to go in sooner

…ally and protect against modification of class attributes
ronaldmaj
ronaldmaj previously approved these changes Sep 2, 2024
Copy link
Collaborator

@ronaldmaj ronaldmaj 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 to me, thanks for putting this together! I tested the ability to change the name string after instantiating it and I'm getting the AttributeError, so all seems to be working 👍

@treefern
Copy link
Collaborator Author

treefern commented Sep 2, 2024

Updated to more robustly work as static classes, preventing modification of class variables. Some other tidying. Addition of unit tests.
This should be ready to go in now.

@treefern
Copy link
Collaborator Author

treefern commented Sep 2, 2024

A release including this PR, published as 0.0.56.dev3 will be required to enable upstream usages to be rolled out.

Copy link
Collaborator

@ronaldmaj ronaldmaj left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for putting in the unit tests as well, this will make this a lot more robust to chances in future. Happy for this to go in 👍

@ronaldmaj ronaldmaj merged commit ad7d68d into main Sep 3, 2024
1 check passed
@treefern treefern deleted the NPI-3458-implement-solution-types-utility-classes branch September 9, 2024 02:14
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