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

Corrected raise Error as e syntax #1566

Merged
merged 2 commits into from
Mar 27, 2019
Merged

Corrected raise Error as e syntax #1566

merged 2 commits into from
Mar 27, 2019

Conversation

alongd
Copy link
Member

@alongd alongd commented Mar 26, 2019

Corrected all instances of raise Error, e: to the correct syntax raise Error as e:. The former won't be supported in Py3.
Also, if e isn't used, it was deleted.
See https://www.python.org/dev/peps/pep-0008/#id51

@alongd alongd added the Py3 label Mar 26, 2019
@alongd alongd self-assigned this Mar 26, 2019
@alongd alongd requested a review from mliu49 March 26, 2019 15:03
Copy link
Contributor

@mliu49 mliu49 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! There are a couple of instances where we could probably get rid of the e variable by replacing raise e with raise, which I did not comment individually on.

On a semi-related note, you also updated external/cinfony/webel.py. First of all, it doesn't seem like we are using this any longer, so I think we could remove it. If we do want to use it in the future (or find some usage that I missed), we should switch to the current version (at https://github.com/cinfony/cinfony) and possibly create a conda package instead.

@@ -354,7 +354,7 @@ def makeNewSpecies(self, object, label='', reactive=True, checkForExisting=True)
try:
spec = Species(index=speciesIndex, label=label, molecule=[molecule], reactive=reactive,
thermo=object.thermo, transportData=object.transportData)
except AttributeError, e:
except AttributeErro:
Copy link
Contributor

Choose a reason for hiding this comment

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

Letter missing.

@@ -1952,7 +1952,7 @@ def getSpecies(self, obj):
try:
spc = self.indexSpeciesDict[obj]
return spc
except KeyError, e:
except KeyError as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an unnecessary try/except clause...

@alongd
Copy link
Member Author

alongd commented Mar 26, 2019

OK, let me know if that's OK and I'll squash

@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #1566 into master will increase coverage by <.01%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1566      +/-   ##
==========================================
+ Coverage   41.85%   41.85%   +<.01%     
==========================================
  Files         165      165              
  Lines       28007    28004       -3     
  Branches     5713     5713              
==========================================
  Hits        11721    11721              
+ Misses      15497    15494       -3     
  Partials      789      789
Impacted Files Coverage Δ
rmgpy/reaction.py 0% <0%> (ø) ⬆️
rmgpy/reduction/input.py 15.62% <0%> (ø) ⬆️
rmgpy/data/base.py 49.59% <0%> (ø) ⬆️
rmgpy/species.py 0% <0%> (ø) ⬆️
rmgpy/tools/regression.py 29.76% <0%> (ø) ⬆️
rmgpy/scoop_framework/framework.py 29.87% <0%> (ø) ⬆️
rmgpy/qm/qmdata.py 85.29% <0%> (ø) ⬆️
rmgpy/qm/symmetry.py 80.48% <0%> (ø) ⬆️
rmgpy/molecule/resonance.py 0% <0%> (ø) ⬆️
rmgpy/qm/molecule.py 80.18% <0%> (ø) ⬆️
... and 7 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 83ad0f2...fabe2b9. Read the comment docs.

@mliu49
Copy link
Contributor

mliu49 commented Mar 26, 2019

Thanks! The fixups look good.

@alongd
Copy link
Member Author

alongd commented Mar 26, 2019

Should be good to go

Copy link
Contributor

@mliu49 mliu49 left a comment

Choose a reason for hiding this comment

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

Thanks! I think this looks good.

This will likely result in conflicts with other PRs, but they should be easily resolvable.

@mliu49 mliu49 merged commit 0648b7d into master Mar 27, 2019
@mliu49 mliu49 deleted the as_e branch March 27, 2019 22:26
@mliu49 mliu49 mentioned this pull request May 15, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants