-
Notifications
You must be signed in to change notification settings - Fork 230
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
Move parse_command_line_arguments to util.py #1717
Conversation
This PR can be reviewed, but let's hold off on merging this for now |
Codecov Report
@@ Coverage Diff @@
## master #1717 +/- ##
=======================================
Coverage 41.66% 41.66%
=======================================
Files 176 176
Lines 30215 30215
Branches 6256 6256
=======================================
Hits 12588 12588
Misses 16703 16703
Partials 924 924
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1717 +/- ##
=======================================
Coverage 32.65% 32.65%
=======================================
Files 87 87
Lines 26158 26158
Branches 6874 6874
=======================================
Hits 8541 8541
+ Misses 16656 16645 -11
- Partials 961 972 +11
Continue to review full report at Codecov.
|
Hi @amarkpayne. I am sorry that I just see this PR. I will have a quick review by today, and please let me know when it is okay to be merged. Thanks! |
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.
@amarkpayne Thanks for the contribution. There is only one minor comment.
rmgpy/rmg/rmgTest.py
Outdated
@@ -42,6 +42,7 @@ | |||
from rmgpy.data.base import ForbiddenStructures | |||
|
|||
from rmg import * |
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.
Do we still need 'from rmg import *?After changing,
main()is the only function imported and is not used in
rmgTest.py`
rmg.py is not installed as a module in the conda binary, so we cannot import this function
a12dc4e
to
0bef99b
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.
I think this looks good. Once it's merged, I think you can cherry-pick it to the v2.5.0 branch as well.
rmg.py is not installed as a module in the conda binary, so we cannot import this function
Motivation or Problem
Fixes #1716
Description of Changes
Move
parse_commnad_line_arguments
inside of rmgpy.util where it can be imported for binary installations