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

chore: reformat codebase with longer line length #2362

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Nov 11, 2024

This is a draft PR to investigate the utility of contributing and maintaining code with a longer line length limits.

The codebase currently has a line length limit of 79 (via #954) as suggested in PEP 8 (which is related to a 80 char limit of decades old IBM punch cards and terminals). However, longer line lengths are generally encouraged for modern Python development, which is a language that commonly has four-space indents needed to define logic structures, thus requiring more horizontal space than other programming languages.

Black has a default of 88, which is the same default for Ruff. Is this the ideal length for the flopy codebase? Not sure, but a quick scan of the suggested changes look much better.

Despite the size of this PR, it was quickly automated using these ruff commands (including quick fixes to ISC001):

ruff format
ruff check --select ISC001 --fix
ruff format

There are no functional changes in this PR, thus the code "runs the same". As such, .git-blame-ignore-revs can be amended to ignore these changes with (e.g.) GitHub's blame view (this would be a follow-up PR).

Is Anyone in favor of increasing the line length? If so, does 88 seem reasonable, or some other length?

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 68.58407% with 284 lines in your changes missing coverage. Please review.

Project coverage is 74.2%. Comparing base (acc5a5b) to head (d4e75c0).
Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
flopy/mf6/utils/model_splitter.py 54.3% 42 Missing ⚠️
flopy/mf6/utils/createpackages.py 8.3% 22 Missing ⚠️
flopy/mf6/mfpackage.py 72.9% 20 Missing ⚠️
flopy/export/netcdf.py 33.3% 18 Missing ⚠️
flopy/mf6/data/mfstructure.py 60.9% 16 Missing ⚠️
flopy/discretization/structuredgrid.py 25.0% 15 Missing ⚠️
flopy/mf6/data/mfdatastorage.py 76.4% 12 Missing ⚠️
flopy/mf6/data/mffileaccess.py 75.5% 11 Missing ⚠️
flopy/mf6/data/mfdataarray.py 75.0% 7 Missing ⚠️
flopy/mf6/utils/reference.py 0.0% 7 Missing ⚠️
... and 43 more
Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #2362    +/-   ##
========================================
  Coverage     74.1%   74.2%            
========================================
  Files          292     292            
  Lines        59476   59622   +146     
========================================
+ Hits         44127   44261   +134     
- Misses       15349   15361    +12     
Files with missing lines Coverage Δ
flopy/mf6/modflow/mfems.py 77.7% <ø> (ø)
flopy/mf6/modflow/mfgweadv.py 70.0% <ø> (ø)
flopy/mf6/modflow/mfgwecnd.py 53.5% <ø> (ø)
flopy/mf6/modflow/mfgwectp.py 46.1% <100.0%> (ø)
flopy/mf6/modflow/mfgwedis.py 42.4% <ø> (ø)
flopy/mf6/modflow/mfgwedisu.py 45.4% <ø> (ø)
flopy/mf6/modflow/mfgwedisv.py 42.4% <ø> (ø)
flopy/mf6/modflow/mfgweesl.py 46.1% <100.0%> (ø)
flopy/mf6/modflow/mfgweest.py 52.1% <ø> (ø)
flopy/mf6/modflow/mfgwefmi.py 64.2% <ø> (ø)
... and 176 more

... and 54 files with indirect coverage changes

@wpbonelli
Copy link
Member

I'd vote for the default 88. And there may be other commits in the history we might want to apply .git-blame-ignore-revs to retrospectively.

This triggered a lot of codacy warnings for some reason, maybe things that it missed before? A lot can be ignored but a few look like real issues

@langevin-usgs
Copy link
Contributor

I like the idea of switching to the default of 88. The resulting code is easier to read. Will need to update the create packages code, but that can be done separately as it is currently under development.

@martclanor
Copy link
Contributor

I also like the line length limit of 88. While we’re on this topic, I think it could be beneficial (and in the same spirit of this PR) to review the other Ruff settings as well, particularly the linting rules that are currently set to be ignored.

@wpbonelli
Copy link
Member

I think it could be beneficial (and in the same spirit of this PR) to review the other Ruff settings as well, particularly the linting rules that are currently set to be ignored.

Agreed, we ignored several to make the initial move to ruff easier but imo it makes sense to aim for the defaults.

Maybe we can add flopy/mf6/**/*.py to tool.ruff.lint.per-file-ignores for now, everything else I'd say is fair game

@mwtoews mwtoews marked this pull request as ready for review November 12, 2024 20:27
@mwtoews
Copy link
Contributor Author

mwtoews commented Nov 12, 2024

Agree that the default 88 char length looks ideal, so let's keep it.

I've revised this PR to exclude flopy/mf6/**/*.py from any further formatting and other potential lint rules (only ISC001 for now). The other change is there were a handful of manual fixes for ISC001.

As for enabling some other Ruff checks, agree this is a good idea, but I recommend to choose these carefully with individual PRs with specific aims/fixes.

@wpbonelli
Copy link
Member

rebasing after #2363 should turn codacy green here

wpbonelli added a commit that referenced this pull request Nov 12, 2024
Codacy began detecting a "severe" pylint E1126 invalid sequence index violation in #2362.

The issue pre-existed that PR, I think the format changes just revealed it. We could just ignore it in the web UI but this is probably worth coming back to, to appease linters and type checkers and to make the code easier to reason about.

Relatedly, there is work ongoing to make ruff work with codacy, so hopefully in future we can use ruff ignore syntax for things like this if/when needed
@wpbonelli wpbonelli merged commit 12a3bcd into modflowpy:develop Nov 13, 2024
23 checks passed
wpbonelli pushed a commit to MODFLOW-USGS/modflow6 that referenced this pull request Nov 13, 2024
This PR reformats Python code with a maximum line length of 88 characters to better represent the code. Note that a related repo MODFLOW-USGS/modflow6-examples already uses the default line length of 88.

See modflowpy/flopy#2362 for a related PR with rational and discussion.

This PR was automated using these Ruff commands (including quick fixes to ISC001):

ruff format
ruff check --select ISC001 --fix
ruff format
@mwtoews mwtoews deleted the line-length-88 branch November 13, 2024 17:04
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