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

Black and flake8 #96

Merged
merged 10 commits into from
Jul 7, 2020
Merged

Black and flake8 #96

merged 10 commits into from
Jul 7, 2020

Conversation

JessicaS11
Copy link
Member

Formatted code using black (and added a pre-commit hook to run black). Added a github action to run flake8 and annotate where code doesn't meet standards

Copy link

@fspaolo fspaolo left a comment

Choose a reason for hiding this comment

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

Unfortunately Black doesn't format docstrings and comments. At some point we might want to conform comments to the 88 line-length limit as well.

This is just personal preference but I'll leave it here in case it's useful.

To break long strings and statements (without using 'backslash'):

"There are no error messages, but an Earthdata login token was not successfully generated"

vs.

("There are no error messages, "
"but an Earthdata login token was not successfully generated")

To simplify long conditional statements:

cond1 = ["bounding_box", "polygon"]
cond2 = ["bbox", "Boundingshape"]

assert ext_type in cond1 or ext_type in cond2, "Invalid spatial extent type."

or

msg = "Your search returned no results; try different search parameters"

assert len(self.avail) > 0, msg

@weiji14
Copy link
Member

weiji14 commented Jul 3, 2020

Unfortunately Black doesn't format docstrings and comments. At some point we might want to conform comments to the 88 line-width limit as well.

There's blackdoc which applies black to code in documentation.

@codecov-commenter
Copy link

Codecov Report

Merging #96 into development will decrease coverage by 1.06%.
The diff coverage is 38.87%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development      #96      +/-   ##
===============================================
- Coverage        49.27%   48.21%   -1.07%     
===============================================
  Files               15       15              
  Lines             1029     1062      +33     
  Branches           214      236      +22     
===============================================
+ Hits               507      512       +5     
- Misses             493      518      +25     
- Partials            29       32       +3     
Impacted Files Coverage Δ
icepyx/__init__.py 100.00% <ø> (ø)
icepyx/tests/is2class_query.py 0.00% <0.00%> (ø)
icepyx/core/variables.py 8.37% <3.75%> (-0.55%) ⬇️
icepyx/core/Earthdata.py 19.64% <5.88%> (-0.73%) ⬇️
icepyx/core/is2ref.py 28.78% <13.51%> (ø)
icepyx/core/icesat2data.py 40.90% <21.25%> (-2.45%) ⬇️
icepyx/core/granules.py 25.50% <23.94%> (-0.35%) ⬇️
icepyx/core/APIformatting.py 63.63% <61.11%> (-0.43%) ⬇️
icepyx/core/geospatial.py 75.00% <66.66%> (ø)
icepyx/core/validate_inputs.py 88.88% <88.57%> (-0.86%) ⬇️
... and 12 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 f9d96d1...2371dc2. Read the comment docs.

@JessicaS11
Copy link
Member Author

After discovering the GitHub action to run flake8 was failing, I did some playing around and it should be working now.

@fspaolo I think you raise a few great points. There's a longstanding debate about whether or not black should format docstrings in any way, and if so, how. I think for now that using the numpydoc format gives us some guidance, if not specifics on spaces and newlines...

@weiji14 Thanks for the info about blackdoc - definitely a good tool for us to consider adding!

@JessicaS11 JessicaS11 merged commit 6f8103f into development Jul 7, 2020
@JessicaS11 JessicaS11 deleted the black branch July 7, 2020 19:44
@fspaolo
Copy link

fspaolo commented Jul 7, 2020

@JessicaS11 right, formatting docstrings usually comes with a lot of personal preference... this is one reason why Black is not imposing this (yet). I would say (as a personal rule) when writing docstrings just keep in mind Black's default 88 line length (since we are imposing that for the rest of the code).

weiji14 pushed a commit that referenced this pull request Aug 30, 2020
* add black pre-commit hook and reformat files using black

* add github action workflow with flake8 on PRs
JessicaS11 added a commit that referenced this pull request Sep 10, 2020
* add black pre-commit hook and reformat files using black

* add github action workflow with flake8 on PRs
JessicaS11 added a commit that referenced this pull request Jan 20, 2021
See #98, which was partly undone by #96.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants