-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
@@ -1,29 +1,66 @@ | |||
repos: | |||
- repo: https://github.com/pre-commit/pre-commit-hooks | |||
rev: v5.0.0 | |||
- repo: local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we've liked local pre-commit for dev, but they do not fully play nice with some popular tooling (aka VSCode), so I think it could be easier for new contributers to avoid dealing with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tylanderson Could you give a bit more context on the problems with a local repo for pre-commit? I feel like I've experienced issues with local pre-commit in the past (I also use VSCode), but I haven't had a problem with that in a while, so I'd benefit from a reminder on what they are 🙂 . I do remember problems where the tool versions specified in the pre-commit yaml (via remote repos) were out of sync with the same packages that were in my requirements-dev.txt: I'd end up where formatting with the version of ruff installed in my venv did something different than the version of ruff specified in the pre-commit yaml. I remember this because I still stumble across this problem in repos that do not use a local repo for pre-commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoiding having a version difference between the local venv (which is what vscode and I assume other tools use) and the pre-commit env was the intention with this. I haven't had any problems with this approach and VSCode, so also interested to hear about the problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pjhartzell I believe this issue tracks the issue I had seen in the past, it's a bit non-obvious from what I remember.
I do agree that the benefit of catching formatting, etc early is nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should document that issue in the README -- I think having a consistent environment that doesn't have to be independently maintained and with possibly different default configuration is worth making it easier to use a single IDE that has broken behavior (at least as I see it).
pygeofilter = "^0.2.4" | ||
returns = "^0.23.0" | ||
python = ">=3.12,<4.0" | ||
fastapi = ">=0.115.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't these all include a "but less than next major" condition as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add it for python because some other package had that constraint, and it wouldn't build without also applying it to python.
The argument for only having a floor is that it's up to the downstream user to validate that it works with whatever specific fastapi/etc version they choose for their app, and that we don't want to unnecessarily restrict the version they can use just because we're not sure it will work. We're also not testing this but with one version of any of these libraries, so it's still not guaranteed that it will work with whatever version the downstream chooses even if it's within some reasonable semantic versioning constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have CI run tests on an install with all dependencies pinned to their minimum version and on an install with latest set of dependency versions that resolve. But I think that is out of scope for this ticket.
Related Issue(s):
Proposed Changes:
PR Checklist: