-
Notifications
You must be signed in to change notification settings - Fork 18
Job tags #136
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
api/jobs.py
Outdated
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.
Can we remove this method and use Category.classifier.value, Category.converter.value, etc in lines 57-59?
It seems cleaner to me.
You avoid the string conversion in line 142 also.
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.
Hmm, not a bad idea. I guess it depends more generally on how we'd like to use enums in the code base.
The downside of that approach is that it wouldn't work if the enums aren't string-based, where the current approach would (if I used self.name in that function rather than self.value).
My eventual intention was that if we use enums more, we'd make a small helper class that does it for you, and let it handle the setup. Let me try that once and see how it feels.
|
LGTM just a suggestion |
|
So it turned out, there was a lot better way to make enums 😄 As proposed, I changed |
|
It feels a little weird that we overrides |
|
Cool. We can change that in the future for sure; to me it felt more explicit that you're converting an enum to a string. Open to further discussion. |
|
My point is that converting an enum to a string doesn't seem like a standard operation, except for logging. |
|
Hmm, yeah I see what you're saying. I'm a little more used to type conversion, and more specifically I'm used to enums being types rather than complex objects. And since they are complex objects, we may as well use their properties. I'll probably switch things around next time I touch this code or make a new enum. |
Of interest to @gsfr, @rentzso, and @ryansanford.