Skip to content

Conversation

@Bibo-Joshi
Copy link
Member

Just a suggestion.

Pros:

  • Less maintenance work keeping reqs-dev and .pre-commit in sync
  • Less noise from dependabot
  • pre-commit still works as expected

Cons:

  • People that want to use the command line tools like black and isort don't have the directly availbale via pip install -r reqs-dev.txt. Not sure how many people actually do that though, plus which of them are not familiar enough with our setup to just get the version from the pre-commit config …

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hey! Looks like you edited the (dev) requirements or the pre-commit hooks. I'm just a friendly reminder to keep the pre-commit hook versions in sync with the dev requirements and the additional dependencies for the hooks in sync with the requirements :)

@Poolitzer
Copy link
Member

Poolitzer commented Jun 25, 2022

The workflow also notified on requirements.txt to keep the additional dependencies in sync with the mypy hook, I would probably still include that, but I'm also fine with it fully gone, whatever you prefer. Just wanted to point it out.

Additionally, I would probably add a line in Contributing.rst, saying that one can install the pre-commit runs manually and run them on the files if desired. Much like the black line we have, maybe append to that.

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

but wouldn't this just make dependabot not notify us of any new releases for the deleted dev-reqs? maybe a better option is to ignore minor and patch versions for those?

we can also change the dependabot separator to "-" to conform to our style more.

fwiw, I'm still for automating updating pre-commit's additional_dependencies on dependabot PR's using a custom .yml script, but I don't know if/how that's possible

Copy link
Member

@harshil21 It should be very easy if we cant tell dependabot about it to write our own for that actually.

Copy link
Member

@Poolitzer yeah we can't even tell pre-commit to update them automatically, see pre-commit/pre-commit#1351

@Bibo-Joshi
Copy link
Member Author

The workflow also notified on requirements.txt to keep the additional dependencies in sync with the mypy hook, I would probably still include that, but I'm also fine with it fully gone, whatever you prefer. Just wanted to point it out.

Thanks for the catch! Had overlooked that and will revert acoordingly

Additionally, I would probably add a line in Contributing.rst, saying that one can install the pre-commit runs manually and run them on the files if desired. Much like the black line we have, maybe append to that.

Jup, good idea 👍

but wouldn't this just make dependabot not notify us of any new releases for the deleted dev-reqs?

pre-commit.ci takes care of that - see #3085

we can also change the dependabot separator to "-" to conform to our style more.

With the slash, Pycharm automatically groups the branches in a directory style. I like that very much tbh, but in the end it's just a ide-specific gimmick. I'm fine with either way

fwiw, I'm still for automating updating pre-commit's additional_dependencies on dependabot PR's using a custom .yml script, but I don't know if/how that's possible

that would indeed be nice! I found dependabot/dependabot-core#2040 on this - FYI.
having a workflow that triggers on PRs that change reqs.txt and search-replaces in .pre-commit.yml should probably suffice … Not a top priority IMO though :D

@Bibo-Joshi Bibo-Joshi requested a review from harshil21 July 7, 2022 21:19
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
@Bibo-Joshi Bibo-Joshi merged commit d4b7a2b into master Jul 9, 2022
@Bibo-Joshi Bibo-Joshi deleted the dev-reqs branch July 9, 2022 21:12
@github-actions github-actions bot locked and limited conversation to collaborators Jul 17, 2022
@harshil21 harshil21 added this to the v20.0a3 milestone Jul 23, 2022
@harshil21 harshil21 added the misc label Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants