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

ParmEdConverter: fix NameError when catching NoDataError #2953

Merged
merged 3 commits into from
Oct 30, 2020

Conversation

cbouy
Copy link
Member

@cbouy cbouy commented Sep 24, 2020

Converting to parmed when the AtomGroup doesn't have a resname leads to a NameError: the current code tries to catch the NoDataError but because NoDataError is not imported from exceptions.py it triggers a NameError: name 'NoDataError' is not defined

Changes made in this Pull Request:

  • import NoDataError from exceptions

Is the changelog update needed here ?

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #2953 into develop will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2953      +/-   ##
===========================================
+ Coverage    93.05%   93.09%   +0.03%     
===========================================
  Files          186      186              
  Lines        24613    24663      +50     
  Branches      3187     3197      +10     
===========================================
+ Hits         22904    22959      +55     
+ Misses        1661     1656       -5     
  Partials        48       48              
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/ParmEd.py 90.96% <100.00%> (+4.17%) ⬆️
package/MDAnalysis/transformations/translate.py 95.23% <0.00%> (-4.77%) ⬇️
package/MDAnalysis/topology/ParmEdParser.py 98.44% <0.00%> (-0.05%) ⬇️
util.py 89.47% <0.00%> (ø)
package/MDAnalysis/lib/util.py 90.05% <0.00%> (ø)
package/MDAnalysis/analysis/msd.py 100.00% <0.00%> (ø)
package/MDAnalysis/analysis/pca.py 98.65% <0.00%> (ø)
package/MDAnalysis/analysis/rdf.py 100.00% <0.00%> (ø)
package/MDAnalysis/analysis/dihedrals.py 100.00% <0.00%> (ø)
package/MDAnalysis/analysis/hole2/hole.py 73.19% <0.00%> (ø)
... and 9 more

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 3df4581...a5bf2fe. Read the comment docs.

@lilyminium
Copy link
Member

Oops, thanks for finding this. Could you please add a test as well so the tests will catch it in the future as well?

@orbeckst
Copy link
Member

You don't need to update the CHANGELOG as this just fixes something that hadn't been released yet. (We don't absolutely strictly adhere to what I just said because our release cycles are so stretched out that we need to track bugs in develop and often fix issues that never show up in a released version. But in the case here a CHANGELOG entry seems overkill.)

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Can you please add a test?

Not for testing that your fix worked but rather because it indicates that we didn't test a possible (and perhaps not uncommon) failure mode earlier.

@pep8speaks
Copy link

pep8speaks commented Oct 29, 2020

Hello @cbouy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-10-29 16:15:54 UTC

@cbouy
Copy link
Member Author

cbouy commented Oct 29, 2020

Sorry for the delay and the messed up commits, should be fixed now

@orbeckst orbeckst merged commit dca4fa8 into MDAnalysis:develop Oct 30, 2020
@orbeckst
Copy link
Member

Thank you!

lilyminium pushed a commit to lilyminium/mdanalysis that referenced this pull request Dec 8, 2020
…2953)

* fix NameError when catching NoDataError
* add test + missing imports
lilyminium pushed a commit to lilyminium/mdanalysis that referenced this pull request Dec 9, 2020
…2953)

* fix NameError when catching NoDataError
* add test + missing imports
lilyminium pushed a commit to lilyminium/mdanalysis that referenced this pull request Dec 14, 2020
…2953)

* fix NameError when catching NoDataError
* add test + missing imports
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.

5 participants