Skip to content
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

Rename submodules in rmgpy.tools #1794

Merged
merged 2 commits into from Nov 6, 2019
Merged

Rename submodules in rmgpy.tools #1794

merged 2 commits into from Nov 6, 2019

Conversation

mliu49
Copy link
Contributor

@mliu49 mliu49 commented Oct 29, 2019

Motivation or Problem

The submodules in the tools module are a bit of a mess in terms of naming. The 3.0 release seems like a good opportunity to rename them.

Note that RMG-website imports will need to be updated, as will any other dependencies which use the tools module.

Description of Changes

Rename submodules in rmgpy.tools and update imports in RMG-Py.

  • Generally, changed to short, all lowercase names without underscores.
  • Test files are identified with capital Test.

Reviewing

Any comments or suggestions about the new names are welcome.

xiaoruiDong
xiaoruiDong previously approved these changes Nov 1, 2019
Copy link
Contributor

@xiaoruiDong xiaoruiDong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mliu49 Thanks for the contribution! I think it is a good idea to standardize the module names. The changes (using all-lowercase names) are according to PEP-8 nomenclature, right? @amarkpayne, your thoughts?

@amarkpayne
Copy link
Member

This is correct according to PEP-8 (https://www.python.org/dev/peps/pep-0008/#package-and-module-names).

I think all of this looks good to go, but should we do the same for the scripts folder? The modules in the script folder are all consistent, but use a different convention.

Rebase and I'll approve and merge

@codecov
Copy link

codecov bot commented Nov 6, 2019

Codecov Report

Merging #1794 into master will increase coverage by 0.06%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1794      +/-   ##
==========================================
+ Coverage   42.92%   42.99%   +0.06%     
==========================================
  Files          82       80       -2     
  Lines       21182    21090      -92     
  Branches     5519     5513       -6     
==========================================
- Hits         9093     9067      -26     
+ Misses      11074    11008      -66     
  Partials     1015     1015
Impacted Files Coverage Δ
rmgpy/tools/diffmodels.py 37.71% <ø> (ø)
rmgpy/tools/mergemodels.py 46.34% <ø> (ø)
rmgpy/tools/canteramodel.py 38.87% <ø> (ø)
rmgpy/tools/ckcsvparser.py 5.06% <ø> (ø)
rmgpy/tools/generatereactions.py 30% <ø> (ø)
rmgpy/rmg/main.py 21.62% <0%> (ø) ⬆️
rmgpy/tools/observablesregression.py 8% <100%> (ø)
rmgpy/tools/regression.py 29.76% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01c36ac...6262bec. Read the comment docs.

@mliu49
Copy link
Contributor Author

mliu49 commented Nov 6, 2019

Should be good to go once tests pass.

@amarkpayne amarkpayne merged commit 05f9382 into master Nov 6, 2019
@amarkpayne amarkpayne deleted the rename_mods branch November 6, 2019 23:18
@amarkpayne
Copy link
Member

Thanks for the PR @mliu49 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants