Move red_knot_python_semantic::PythonVersion to the ruff_python_ast crate#16147
Move red_knot_python_semantic::PythonVersion to the ruff_python_ast crate#16147
red_knot_python_semantic::PythonVersion to the ruff_python_ast crate#16147Conversation
ntBre
requested review from
AlexWaygood,
MichaReiser,
carljm,
dhruvmanila and
sharkdp
as code owners
|
| Self { | ||
| source_type: PySourceType::default(), | ||
| target_version: PythonVersion::default(), | ||
| target_version: PyVersion::default(), |
There was a problem hiding this comment.
I think it shouldn't be much effort to use PythonVersion here.
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thanks.
I'd prefer to keep the API boundary Python version types used in the CLI and wasm because I don't want the parser to depend on clap or wasm bindgen (even optionally). It also allows us to add a new python version only to one tool but not the other.
I'd also prefer not to move PyVersion to the parser. I find that confusing. We should keep this type in the linter crate if migrating the linter is too much effort.
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
I agree with unifying these enums/structs to some extent, but it does feel a bit odd to me that we'd have the common type in ruff_python_parser, since it's not really something specific to parsing Python code -- it's a general-purpose type that you could use for lots of things. ruff_python_ast might be a better fit -- or what about ruff_python_stdlib? If we put it there, we could use it in functions like this, which "should really" take a PythonVersion as their argument (it would make the signature much more strongly typed):
I think the disadvantage might be that we'd have to declare an explicit dependency on ruff_python_stdlib from more other crates -- but I doubt that this would be a huge problem in practice, since both Ruff and red-knot already depend on it, and it's a pretty minimal crate that doesn't depend on any other crates in the workspace. Curious for @MichaReiser's thoughts...
|
I don't think it should be |
|
|
This reverts commit 9a35763.
|
Sounds good, I already reverted everything except moving Thanks for the reviews! |
|
|
Co-authored-by: Alex Waygood <[email protected]>
|
Moving the type and doing the unification in separate PRs does make sense to me |
|
Sounds good. I think I just started having some promising serde results, but it makes sense to open a separate PR. I'll push the change Alex recommended and then merge! |
ntBre
changed the title
red_knot_python_semantic::PythonVersion to the ruff_python_ast crate
3 participants
Summary
This PR moves the
PythonVersionstruct from thered_knot_python_semanticcrate to theruff_python_astcrate so that it can be used more easily in the syntax error detection work. Compared to that prototype these changes reduce us from 2PythonVersionstructs to 1.This does not unify any of the
PythonVersionenums, but I hope to make some progress on that in a follow-up.Test Plan
Existing tests, this should not change any external behavior.