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

pre-commit update #1150

Merged
merged 8 commits into from
Jul 11, 2022
Merged

pre-commit update #1150

merged 8 commits into from
Jul 11, 2022

Conversation

PGijsbers
Copy link
Collaborator

Reference Issue

closes #1146
The issue identified problems with the outdated fixed versions of the pre-commit file. For me personally, it lead to different issues, so I decided it's probably best to bump all dependencies to their newest versions.

What does this PR implement/fix? Explain your changes.

Update all dependencies used in pre-commit hooks to their most recent version.
Other files have been updated accordingly to pass the new pre-commit tests, this mainly involves:

  • black formatting of docstrings, which now no longer have a leading and trailing space.
  • black formatting of many comma-separates lists (e.g., when calling a function), which is now by default reformatted to one-item-per-line, as opposed to only using this if it would otherwise exceed line length. (Personally I am a fan)
  • Resolving a few new mypy complaints.

How should this PR be tested?

Not really sure, installing the latest pre-commit hooks and verifying that it works as expected?

PGijsbers added 8 commits July 1, 2022 16:43
Black was bumped from 19.10b0 to 22.6.0. Changes in the files are
reduced to:
 - No whitespace at the start and end of a docstring.
 - All comma separated "lists" (for example in function calls) are now
   one item per line, regardless if they would fit on one line.
@PGijsbers PGijsbers requested a review from mfeurer July 1, 2022 15:37
@PGijsbers PGijsbers added the CI About continious integration label Jul 1, 2022
Copy link
Collaborator

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

Great PR. Do you know if there's a way we could automate these updates of black etc.?

Copy link
Collaborator

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

Sorry, this was supposed to be an approve.

@PGijsbers
Copy link
Collaborator Author

PGijsbers commented Jul 11, 2022

Do you know if there's a way we could automate these updates of black etc.?

The reason we use fixed versions is to make sure that everyone has the same environment because, as you see from this PR, rules can change between different versions. Second, since rules can change between versions, as I see it automated version bumps would mean either:

  • updating only the pre-commit file, leading to a state where the repository might not pass its pre-commit checks
  • updating the pre-commit file and auto-formatting (works for black, but mypy issues can't be automatically resolved). Force merging these changes means automatically accepting whichever changes black makes (not a fan, should probably be evaluated by us before accepting the new style guide). Doing it through an automated PR means that effectively very little work is actually automated (a version bump and formatting for review by us), doing this for every release of black/mypy might actually lead to more work?

So I am not really sure of any useful way to automate this procedure.

edit: as a middle ground perhaps it can be part of a post-release job, where after each major release we have the dependencies checked/updated. But I am still not sure if the time spent automating this is worth it, maybe there's premade github workflows.

@PGijsbers PGijsbers merged commit c6fab8e into develop Jul 11, 2022
@PGijsbers PGijsbers deleted the black-update branch July 11, 2022 08:06
@mfeurer
Copy link
Collaborator

mfeurer commented Jul 11, 2022

Good points. Maybe something like the dependabot which opens PRs to bump the versions, and we can then add to the PRs to fix these issues? But maybe it's not worth pursuing this?

@PGijsbers
Copy link
Collaborator Author

Opened an issue so we can look into it later 👍

@PGijsbers
Copy link
Collaborator Author

(of course, feel free to start looking into this yourself now if you want to)

PGijsbers added a commit to Mirkazemi/openml-python that referenced this pull request Feb 23, 2023
* Update to latest versions

* Updated Black formatting

Black was bumped from 19.10b0 to 22.6.0. Changes in the files are
reduced to:
 - No whitespace at the start and end of a docstring.
 - All comma separated "lists" (for example in function calls) are now
   one item per line, regardless if they would fit on one line.

* Update error code for "print"

Changed in flake8-print 5.0.0: https://pypi.org/project/flake8-print/

* Shorten comment to observe line length codestyle

* Install stubs for requests for mypy

* Add dependency for mypy dateutil type stubs

* Resolve mypy warnings

* Add update pre-commit dependencies notice
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI About continious integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre-commit hook fails due to import error
2 participants