-
Notifications
You must be signed in to change notification settings - Fork 60
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
2-3-Compatibility of Python Code #75
Conversation
Please check the gihub actions for python, you still have several problems |
Also it would be good to install the python stuff under |
Agreed, as there are actually a few issues to fix with the python code organization. I'd propose I do it in a separate PR. |
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.
Thanks for quickly taking care of this.
Not sure if this was using an automated tool or by hand?
I think we don't have to use six either otherwise we should ship it to make sure it exists, and I made proposals inline to that effect, though maybe I missed some cases.
Also some other proposals to do change some code for niceness
Co-Authored-By: Andre Sailer <andre.philippe.sailer@cern.ch>
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 to me
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.
You still did not resolved 2 vs 4 indent or what are we doing about this?
setup.cfg
Outdated
# line break after binary operator (contradicts W503) | ||
W504 | ||
# module level import not at top of file (poblem with ROOT) | ||
E402 |
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.
E402 what if you just flag the lines of ROOT as false positives
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.
Done!
@petricm good point, I just switched everything to 4 spaces now. |
As discussed in the EDM4hep meeting, I changed this to 2 spaces indent. @gaede should be good to merge now! |
BEGINRELEASENOTES
ENDRELEASENOTES