Python Linting With `black` #358

TimothyWillard · 2024-10-24T14:48:33Z

Describe your changes.

This pull request:

Adds a GitHub action to use for code linting. For now only applies the black formatter to python code, but general enough that it could be expanded in the future (i.e. using more detailed tools like pylint or ruff or linting for other files like yaml, [Feature request]: YAML Linting #343, or R).
Applies the black formatter to the python files contained in the repo. The exact command used was black . --line-length 92 --extend-exclude flepimop/gempyor_pkg/src/gempyor/steps_rk4.py.
This PR also uses the --verbose option with the black formatter, I view this as a temporary thing as we transition to formatting code and as folks get used to it we can then get rid of it since it is a bit too much output for a GitHub action.

Open questions:

What do we think of the changed files, aesthetically do they look good?
I'll update the docs with whatever we end up settling on, but are there better ways to encourage use? Git pre-commit hooks are an option but then have to figure out how to support windows, could use a package for managing those like pre-commit but that requires a new tool, or could write some pre-push script but then have to maintain the same actions in more than one place. Thoughts (can also push to an issue if too gnarly for this PR)?
I excluded the steps_rk4.py that was mentioned by name in the previous PR Initial python linting #282 (comment). I'm sympathetic to the fact that this file is hard to read when line lengths are shorter, but I think this is a sign that it needs to be refactored at some point which is beyond the scope of this issue. For now we can skip it and then address in a dedicated issue?

What does your pull request address? Tag relevant issues.

This pull request starts to address GH-279 and replaces GH-282 (I was concerned about making the right git maneuvers to get that into shape since it is over two months old so opting to close instead).

Tag relevant team members.

@emprzy @fang19911030 @jcblemai @pearsonca @saraloo @shauntruelove

Added an initial GitHub action for linting, currently only contains black formatter for python, but general enough for other linting in the future.

The exact command ran was `black . --line-length 92 --extend-exclude flepimop/gempyor_pkg/src/gempyor/steps_rk4.py`.

pearsonca

code formatting looks fine - one note about the action itself in comment below (for other's to chime in on as well).

One additional ask: can we have an "easy mode" either script or documentation update about how to apply the check on the local end?

pearsonca · 2024-10-24T15:37:10Z

.github/workflows/lint.yml

+on:
+  workflow_dispatch:
+  push:
+    paths:
+      - '**/*.py'
+    branches:
+      - main
+  pull_request:
+    paths:
+      - '**/*.py'
+    branches:
+      - main


what do we think about restricting this to the package materials, but expanding it to more branches?

I definitely want less non-library code in the repo moving forward, and linting is generally good, but ... seems like it might be an unnecessary headache for operations for the library-adjacent code, while letting too much mess accumulate in library for the operational branches that we want to merge back in.

pearsonca · 2024-10-24T16:53:29Z

code formatting looks fine - one note about the action itself in comment below (for other's to chime in on as well).

One additional ask: can we have an "easy mode" either script or documentation update about how to apply the check on the local end?

So to offer opinion on open Qs:

Yes
pre-commit hook works for me - probably fine to ignore development on windows machines. is the plan here to have linting be potentially merge-blocking as well? if that's the case, everyone can still see the CI, just a bit more work for the already more annoying dev platform to contribute
yep, separate issue. I remain slighly agog that we have our own RK4 implementation at all.

TimothyWillard · 2024-10-25T15:34:39Z

Offline discussion between @jcblemai, @pearsonca, and I:

Pre-commit hook is good and simple, can support Mac+Linux first and if needed add Windows support in a separate issue (will require assistance of someone who knows windows dev).
Want to trigger the action on:
- Pushes to any branch for files contained in a package (only gempyor at this point) for quick feedback while folks work.
- Pull requests to main for all python files.
It's important to note that linting does not block pushing/merging, that would be at reviewer discretion, but provides strong encouragement.

Action now triggers on a push to any branch that modifies gempyor for quick feedback and pull request with any modified python files.

Added a quick pre-commit hook that will apply the black formatter with the same params used in the GitHub action.

Updated the documentation to reflect the new usage of `black` and how to use the pre commit git hook.

pearsonca

Looks good. Doc add re process is nice.

In anticipation of GH-358, minimize future work.

MacdonaldJoshuaCaleb

This is a really nice quality of life feature, thanks for working on this

@fang19911030

Add a utility per @fang19911030 request to apply `black` with the same settings that the GitHub action uses.

saraloo

Thanks, this looks good to me. Just noting agreement that we should adapt something similar for R code too later down the line.

In anticipation of GH-358, minimize future work.

TimothyWillard requested review from emprzy, jcblemai, fang19911030, pearsonca, shauntruelove and saraloo October 24, 2024 14:48

TimothyWillard mentioned this pull request Oct 24, 2024

Initial python linting #282

Closed

TimothyWillard added 2 commits October 24, 2024 10:56

Code linting GitHub action

b15e29e

Added an initial GitHub action for linting, currently only contains black formatter for python, but general enough for other linting in the future.

Apply black formatter

3331eb8

The exact command ran was `black . --line-length 92 --extend-exclude flepimop/gempyor_pkg/src/gempyor/steps_rk4.py`.

TimothyWillard force-pushed the GH-279/linting-with-black branch from 6f9bd15 to 3331eb8 Compare October 24, 2024 14:57

TimothyWillard added enhancement Request for improvement or addition of new feature(s). gempyor Concerns the Python core. low priority Low priority. quick issue Short or easy fix. labels Oct 24, 2024

pearsonca reviewed Oct 24, 2024

View reviewed changes

jcblemai previously approved these changes Oct 25, 2024

View reviewed changes

TimothyWillard added 3 commits October 25, 2024 13:55

Change lint action triggers

d310fed

Action now triggers on a push to any branch that modifies gempyor for quick feedback and pull request with any modified python files.

Added lint pre-commit hook

1752ce1

Added a quick pre-commit hook that will apply the black formatter with the same params used in the GitHub action.

Merge main into GH-279/linting-with-black

cbee39b

TimothyWillard dismissed jcblemai’s stale review via cbee39b October 25, 2024 18:18

Add new formatter updates to the GitBook

b144b90

Updated the documentation to reflect the new usage of `black` and how to use the pre commit git hook.

TimothyWillard requested review from pearsonca and jcblemai October 25, 2024 19:15

pearsonca previously approved these changes Oct 25, 2024

View reviewed changes

jcblemai previously approved these changes Oct 25, 2024

View reviewed changes

Skip for draft PRs

8e67ca8

TimothyWillard dismissed stale reviews from jcblemai and pearsonca via 8e67ca8 October 30, 2024 16:10

jcblemai previously approved these changes Oct 30, 2024

View reviewed changes

emprzy previously approved these changes Oct 30, 2024

View reviewed changes

TimothyWillard added a commit that referenced this pull request Oct 31, 2024

Reformat new files with line length 92

7ffa3f3

In anticipation of GH-358, minimize future work.

TimothyWillard requested review from MacdonaldJoshuaCaleb and twallema November 1, 2024 14:51

MacdonaldJoshuaCaleb previously approved these changes Nov 1, 2024

View reviewed changes

Add bin/lint utility

71ee9b1

Add a utility per @fang19911030 request to apply `black` with the same settings that the GitHub action uses.

TimothyWillard dismissed stale reviews from MacdonaldJoshuaCaleb, emprzy, and jcblemai via 71ee9b1 November 1, 2024 15:24

TimothyWillard requested review from emprzy, MacdonaldJoshuaCaleb, jcblemai and pearsonca November 1, 2024 15:32

emprzy approved these changes Nov 1, 2024

View reviewed changes

jcblemai approved these changes Nov 1, 2024

View reviewed changes

saraloo approved these changes Nov 4, 2024

View reviewed changes

TimothyWillard merged commit 1bd4a94 into main Nov 4, 2024
3 checks passed

TimothyWillard deleted the GH-279/linting-with-black branch November 4, 2024 17:56

TimothyWillard mentioned this pull request Nov 4, 2024

Release cycle, December 2024 #375

Merged

TimothyWillard added a commit that referenced this pull request Nov 4, 2024

Reformat new files with line length 92

0e69303

In anticipation of GH-358, minimize future work.

TimothyWillard added a commit that referenced this pull request Nov 8, 2024

Reformat new files with line length 92

498197c

In anticipation of GH-358, minimize future work.

This was referenced Nov 15, 2024

[Feature request]: Add pylint Or ruff Linting For gempyor #398

Open

Add Python Code Linting For gempyor #279

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python Linting With `black` #358

Python Linting With `black` #358

TimothyWillard commented Oct 24, 2024 •

edited

Loading

pearsonca left a comment

pearsonca Oct 24, 2024

pearsonca commented Oct 24, 2024

TimothyWillard commented Oct 25, 2024

pearsonca left a comment

MacdonaldJoshuaCaleb left a comment

saraloo left a comment

Python Linting With black #358

Python Linting With black #358

Conversation

TimothyWillard commented Oct 24, 2024 • edited Loading

Describe your changes.

What does your pull request address? Tag relevant issues.

Tag relevant team members.

pearsonca left a comment

Choose a reason for hiding this comment

pearsonca Oct 24, 2024

Choose a reason for hiding this comment

pearsonca commented Oct 24, 2024

TimothyWillard commented Oct 25, 2024

pearsonca left a comment

Choose a reason for hiding this comment

MacdonaldJoshuaCaleb left a comment

Choose a reason for hiding this comment

saraloo left a comment

Choose a reason for hiding this comment

Python Linting With `black` #358

Python Linting With `black` #358

TimothyWillard commented Oct 24, 2024 •

edited

Loading