fix: target-version fallback with extend#21980
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| invalid-syntax: | 15 | 15 | 0 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+15 -0 violations, +0 -0 fixes in 3 projects; 52 projects unchanged)
facebookresearch/chameleon (+8 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --no-fix --output-format concise --preview
+ chameleon/inference/chameleon.py:648:19: invalid-syntax: Cannot use named assignment expression (`:=`) on Python 3.7 (syntax was added in Python 3.8) + chameleon/miniviewer/miniviewer.py:190:16: invalid-syntax: Cannot use named assignment expression (`:=`) on Python 3.7 (syntax was added in Python 3.8) + chameleon/viewer/backend/models/chameleon_distributed.py:405:13: invalid-syntax: Cannot use `match` statement on Python 3.7 (syntax was added in Python 3.10) + chameleon/viewer/backend/models/chameleon_distributed.py:818:17: invalid-syntax: Cannot use `match` statement on Python 3.7 (syntax was added in Python 3.10) + chameleon/viewer/backend/models/chameleon_local.py:633:13: invalid-syntax: Cannot use `match` statement on Python 3.7 (syntax was added in Python 3.10) + chameleon/viewer/backend/models/service.py:131:21: invalid-syntax: Cannot use `match` statement on Python 3.7 (syntax was added in Python 3.10) + chameleon/viewer/backend/models/service.py:154:17: invalid-syntax: Cannot use `match` statement on Python 3.7 (syntax was added in Python 3.10) + chameleon/viewer/backend/models/service.py:226:25: invalid-syntax: Cannot use `match` statement on Python 3.7 (syntax was added in Python 3.10)
qdrant/qdrant-client (+2 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --no-fix --output-format concise --preview
+ qdrant_client/async_qdrant_fastembed.py:226:16: invalid-syntax: Cannot use named assignment expression (`:=`) on Python 3.7 (syntax was added in Python 3.8) + qdrant_client/qdrant_fastembed.py:223:16: invalid-syntax: Cannot use named assignment expression (`:=`) on Python 3.7 (syntax was added in Python 3.8)
openai/openai-cookbook (+5 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select A,E703,F704,B015,B018,D100
+ examples/Context_summarization_with_realtime_api.ipynb:cell 15:7:16: invalid-syntax: Cannot use named assignment expression (`:=`) on Python 3.7 (syntax was added in Python 3.8) + examples/Embedding_long_inputs.ipynb:cell 12:9:12: invalid-syntax: Cannot use named assignment expression (`:=`) on Python 3.7 (syntax was added in Python 3.8) + examples/agents_sdk/session_memory.ipynb:cell 42:30:53: invalid-syntax: Cannot use named assignment expression (`:=`) on Python 3.7 (syntax was added in Python 3.8) + examples/chatgpt/rag-quickstart/azure/Azure_AI_Search_with_Azure_Functions_and_GPT_Actions_in_ChatGPT.ipynb:cell 21:7:12: invalid-syntax: Cannot use named assignment expression (`:=`) on Python 3.7 (syntax was added in Python 3.8) + examples/chatgpt/rag-quickstart/gcp/Getting_started_with_bigquery_vector_search_and_openai.ipynb:cell 18:7:12: invalid-syntax: Cannot use named assignment expression (`:=`) on Python 3.7 (syntax was added in Python 3.8)
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| invalid-syntax: | 15 | 15 | 0 | 0 | 0 |
a53b9bf to
e35fa57
Compare
crates/ruff/tests/cli/snapshots/cli__lint__requires_python_extend_from_shared_config.snap
Show resolved
Hide resolved
|
Thanks for looking into this. Would you mind taking some time and update your summary with an explanation:
|
|
For what it is worth, I tried this MR (at commit e35fa57) with my reproducer example described in #21956 and it behaved as desired, using the I also tried the following scenarios:
I only tested variations around my very minimal example, but they all behaved as expected. Thank you. |
Damn, thank you for testing! |
dylwil3
left a comment
There was a problem hiding this comment.
This looks correct to me, and much nicer than the original incorrect implementation 😄
The two things I'd love to see are:
- A verification that the ecosystem changes are as expected (let me know if you need help with this)
- Have you done any testing in VSCode to see whether we need changes to
ruff_server? See some of the discussion later in the original implementation here #16319 IIRC there was some work to ensure that theruff_serverconfiguration resolution was compatible with the CLI.
crates/ruff/tests/cli/snapshots/cli__lint__requires_python_extend_from_shared_config.snap
Show resolved
Hide resolved
|
On the VSCode test, after you question I tested with next setup by comparing the released v0.14.8 with my local build that includes this patch. Test setup:
Before (released v0.14.8): After (local build + fix): |
|
I wasn't able to locate the exact config files in these repos (may they be using chains or the ecosystem tests may inject configs?). However, the violations are clearly legitimate - the code uses modern Python syntax (walrus operators, match statements) that's incompatible with a Python 3.7 target version somewhere in their config chain. The fix is correctly detecting these incompatibilities that were previously hidden. facebookresearch/chameleon is archived though |
e35fa57 to
1638dd5
Compare
dylwil3
left a comment
There was a problem hiding this comment.
Thank you and sorry for the long delay in landing this!
I tried to find a way to put the ConfigurationOrigin::UserSettings fallback logic (used in both the CLI and the editor) also inside of your apply_fallbacks, but I don't see a quick way to do it without further refactoring. I added a comment instead in case someone wants to help make that nicer another day.
280488a to
65823b5
Compare
## Summary Closes #21956 **Root cause:** When a .ruff.toml, ruff.toml config was discovered via ancestor search, Ruff eagerly derived target-version from requires-python in pyproject.toml during config loading. That fallback value then participated in config merging and incorrectly overrode an explicit target-version defined in an extended config. **Fix:** Defer the requires-python fallback until after the full extend chain across .ruff.toml / ruff.toml is merged, and apply it only if target-version is still unset. **Impact:** Explicit target-version settings in extended configs are now respected, requires-python is only used as a fallback. ## Test Plan Added a new test. I was useful to validate the fix but may be redundant to keep, lmk if you would love me to remove it --------- Co-authored-by: dylwil3 <dylwil3@gmail.com>
## Summary Closes #21956 **Root cause:** When a .ruff.toml, ruff.toml config was discovered via ancestor search, Ruff eagerly derived target-version from requires-python in pyproject.toml during config loading. That fallback value then participated in config merging and incorrectly overrode an explicit target-version defined in an extended config. **Fix:** Defer the requires-python fallback until after the full extend chain across .ruff.toml / ruff.toml is merged, and apply it only if target-version is still unset. **Impact:** Explicit target-version settings in extended configs are now respected, requires-python is only used as a fallback. ## Test Plan Added a new test. I was useful to validate the fix but may be redundant to keep, lmk if you would love me to remove it --------- Co-authored-by: dylwil3 <dylwil3@gmail.com>
Summary
Closes #21956
Root cause: When a .ruff.toml, ruff.toml config was discovered via ancestor search, Ruff eagerly derived target-version from requires-python in pyproject.toml during config loading. That fallback value then participated in config merging and incorrectly overrode an explicit target-version defined in an extended config.
Fix: Defer the requires-python fallback until after the full extend chain across .ruff.toml / ruff.toml is merged, and apply it only if target-version is still unset.
Impact: Explicit target-version settings in extended configs are now respected, requires-python is only used as a fallback.
Test Plan
Added a new test. I was useful to validate the fix but may be redundant to keep, lmk if you would love me to remove it