-
Notifications
You must be signed in to change notification settings - Fork 154
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
Linting update #1131
Linting update #1131
Conversation
Test: test_2_rounds_1k_duckdbPercentage change: -12.1%
Test: test_2_rounds_1k_sqlitePercentage change: -3.4%
Click here for vega lite time series charts |
My understanding is that it is considered best practice to pin versions for development and CI, and ideally these two versions are also the same. You already use poetry, so I'd install from the lockfile in CI. IF installing all of the dev deps are too slow for the linting task (but I'd guess this would be fast), then you could split the dev deps into |
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.
@ADBond I'm approving this as is, but also happy if you think we should do something similar to @NickCrews's suggestion on this branch and re-review later.
@NickCrews thanks for suggestion - think timings should be reasonable as we can cache the dependencies, so should only be slow once. Had been wondering about sorting out some dev groups for a bit for e.g. building docs, so may see about adding that in. @RossKen cheers for that - as this is pretty thin as-is and not super pressing I might implement the above and pin the linter version in this PR to avoid similar issues. |
…nstalling poetry itself
this keeps the versions of ruff and black in sync with pyproject.toml - then we only have a single location specifying the canonical versions of these packages
…k packages in testing env
@RossKen I have updated this PR now - have outlined the additional changes as in edit to initial comment on this PR ☝️ for visibility. Let me know if you have any thoughts/suggestions |
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.
Looks sensible from my side, and tests run pretty quickly too so happy to merge 👍
Add explicit
stacklevel
kwarg forwarnings.warn()
as per latest ruff release (v0.0.257) - see notes and in particular this change.As a slight side-point, I wonder if it is worth pinning a ruff version in our CI if there are going to be frequent breaking changes like this, and occasionally bumping version and doing a bigger linting update? or does anyone think it's preferable to just make frequent small updates like this (or adding exclusion rules if appropriate)?
edit:
from discussion I have now pinned versions of ruff and black in
pyproject.toml
, and workflows install from this, to synchronise versions between CI and local dev.Additionally I have split dev dependencies into some meaningful groups - the only practical difference is now that the
benchmarking
dependencies are opt-in, and not installed by default. In CI I have separated virtual environments for linting, testing, and benchmarking - any updates topoetry.lock
will update them all, regardless of whether the change is relevant, owing to limitations with how poetry handles dependency groups. I figure that this happens fairly rarely, and so will probably not have a large impact on speed in most cases.