-
Couldn't load subscription status.
- Fork 1
use enum for user class #438
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good in general, thanks! ... What could be improved is mainly three-fold:
- The random user class generation for tests should stay as is and not be hard-coded.
- UserClass so far has had a variant 0 = NA (not available).
- I'm not a fan of panicking when not able to parse an integer. Either this should be handled explicitly using TryFrom or mapping to variant NA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Left a couple comments, did not comment on the UserClass stuff again, since that can be handled in the other PR.
5315882 to
9997ad7
Compare
9997ad7 to
e691e78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Thanks a lot! :)
|
Just fixed a tiny detail ... moved UserClass to the submodule project since it is actually a property of a project rather than a user ... i know the name is a bit confusing. |
Signed-off-by: Sandro-Alessio Gierens <[email protected]>
d3aff6a to
e8522c1
Compare
close #425