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

modernize formatting code for python 3.6+ #944

Merged
merged 4 commits into from
Feb 7, 2019

Conversation

kosack
Copy link
Contributor

@kosack kosack commented Jan 31, 2019

Since we dropped support for Python 3.5, we can now use some of the features of 3.6 everywhere. This PR upgrades the use of str.format to use f-strings instead (cleaner). The edits were not made manually, rather I used the pyupgrade refactoring tool (https://github.com/asottile/pyupgrade)

@codecov
Copy link

codecov bot commented Jan 31, 2019

Codecov Report

Merging #944 into master will not change coverage.
The diff coverage is 63.82%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #944   +/-   ##
=======================================
  Coverage   78.86%   78.86%           
=======================================
  Files         191      191           
  Lines       10966    10966           
=======================================
  Hits         8648     8648           
  Misses       2318     2318
Impacted Files Coverage Δ
ctapipe/utils/CutFlow.py 94.91% <ø> (ø) ⬆️
ctapipe/core/traits.py 95.65% <ø> (ø) ⬆️
ctapipe/io/toymodel.py 0% <ø> (ø) ⬆️
ctapipe/image/muon/muon_reco_functions.py 66.66% <0%> (ø) ⬆️
ctapipe/tools/extract_charge_resolution.py 73.91% <0%> (ø) ⬆️
ctapipe/io/targetioeventsource.py 14.72% <0%> (ø) ⬆️
ctapipe/io/array.py 50% <0%> (ø) ⬆️
ctapipe/tools/bokeh/file_viewer.py 72.33% <0%> (ø) ⬆️
ctapipe/coordinates/representation.py 80.55% <0%> (ø) ⬆️
ctapipe/core/container.py 67.32% <0%> (ø) ⬆️
... and 29 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 37f70ce...4daeef9. Read the comment docs.

@kosack
Copy link
Contributor Author

kosack commented Feb 1, 2019

This is pretty uncontroversial I think and ready for review - the poor codecov is unrelated - it just detects all of the coverage problems in the affected files

dneise
dneise previously approved these changes Feb 1, 2019
@kosack
Copy link
Contributor Author

kosack commented Feb 1, 2019

ah, it seems codacy still is not treating this as modern python code (hence it now complains about the use of f-strings). I think we can ignore that for now and I'll try to see if there is a way around it (they claim it's fixed https://github.com/codacy/codacy-pylint/issues/41 , but I seems not to be)

@kosack
Copy link
Contributor Author

kosack commented Feb 1, 2019

It seems no option I try makes codacy understand that this is python3.6. I could simply disable Prospector, perhaps. Even disabling the error F999 seems to have no effect.

@kosack
Copy link
Contributor Author

kosack commented Feb 1, 2019

I'll contact their support, but for now let's just accept this as is.

@kosack
Copy link
Contributor Author

kosack commented Feb 6, 2019

I received an answer from Codacy, and it seems they are still using an outdated version of Prospector that doesn't support python 3.6+ syntax. They suggested just disabling that pattern (Prospector_F999), so I did so. Otherwise this PR is ready to go

@kosack kosack requested a review from maxnoe February 7, 2019 14:15
@dneise dneise merged commit e638142 into cta-observatory:master Feb 7, 2019
@kosack kosack deleted the modernize_code branch May 20, 2019 14:03
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.

3 participants