-
Notifications
You must be signed in to change notification settings - Fork 10
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
WIP: Add generic friends to KingMaker, refactor tasks, rework building of tarballs #22
Conversation
It looks good to me |
processor/tasks/CROWNBase.py
Outdated
# sanitize the scopes information | ||
try: | ||
self.scopes = ast.literal_eval(str(self.scopes)) | ||
except: |
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.
Wouldn't it be beneficial to state the expected errors explicitly/use contextlib.suppress
if no change is applied in case of occurring Exception?
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.
I rewrote the function to be much simpler :D
processor/tasks/CROWNBase.py
Outdated
# sanitize the shifts information | ||
try: | ||
self.shifts = ast.literal_eval(str(self.shifts)) | ||
except: |
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.
Wouldn't it be beneficial to state the expected errors explicitly/use contextlib.suppress if no change is applied in case of occurring Exception?
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.
s.o.
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.
It looks fine to me as well (in addition to the minor comments mentioned above)
If you are happy with the PR, please set an approve @conformist89 @a-monsch |
No description provided.