Upgrade RustPython #5192
Upgrade RustPython #5192
Conversation
charliermarsh
force-pushed
the
charlie/upgrade
branch
from
870be5b to
cb19936
Compare
charliermarsh
force-pushed
the
charlie/upgrade
branch
2 times, most recently
from
3984ae6 to
9e01948
Compare
There was a problem hiding this comment.
Thank you!
This looks good to me. I am a bit concerned about all the new dependencies that the upgrade introduces and wonder if it could make sense to add helpers to Arguments to avoid the many chain in the user code (get all defaults, get all named arguments, get all positional arguments, get all arguments) etc.
Cargo.lock
Outdated
| @@ -198,6 +198,22 @@ version = "2.3.1" | |||
| source = "registry+https://github.com/rust-lang/crates.io-index" | |||
| checksum = "6776fc96284a0bb647b615056fc496d1fe1644a7ab01829818a6d91cae888b84" | |||
|
|
|||
| [[package]] | |||
| name = "block-buffer" | |||
There was a problem hiding this comment.
So many new dependencies. Are they all introduced because of the bigint library migration? Would it make sense for us to stay on numbigint for now?
There was a problem hiding this comment.
I'm not sure, but we are staying on num-bigint, aren't we? I'm not using the malachite feature.
There was a problem hiding this comment.
It does look like block-buffer comes from malachite. But does this actually affect the binary? I can also just remove the malachite stuff in a separate PR.
There was a problem hiding this comment.
Oh, I think rustpython_format doesn't have this properly gated as a feature. I'll fix this separately.
There was a problem hiding this comment.
Ok, fixed this. No new deps.
| for arg in &args.posonlyargs { | ||
| if let Some(expr) = &arg.annotation { | ||
| for arg_with_default in chain!(&args.posonlyargs, &args.args, &args.kwonlyargs) { | ||
| if let Some(expr) = &arg_with_default.def.annotation { |
There was a problem hiding this comment.
Hmm another acronym with def. I should have catched that during the review. I guess that's fine for now.
There was a problem hiding this comment.
Yeah don't love it.
| } in chain!( | ||
| &arguments.posonlyargs, | ||
| &arguments.args, | ||
| &arguments.kwonlyargs |
There was a problem hiding this comment.
What's the motivation for using chain! over iter.chain? I know we have a few use cases for itertools but I try to keep them to a minimum, in the hope, that we could someday remove the dependency.
There was a problem hiding this comment.
I can change it to iter.chain. I just wanted to be consistent everywhere and this was more concise.
| def, | ||
| default: _, | ||
| range: _, | ||
| } in chain!(&arguments.posonlyargs, &arguments.args) |
There was a problem hiding this comment.
Should we add some custom methods to arguments that allow you iterating over all arguments, over all defaults, etc? It feels very repetitive to have these chain calls sprinkled across the code base.
There was a problem hiding this comment.
Yeah, maybe, I'm gonna punt it on now under "leave it better" (it's no worse than before).
crates/ruff/src/rules/pylint/rules/unexpected_special_method_signature.rs
Outdated
Show resolved
Hide resolved
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
charliermarsh
force-pushed
the
charlie/upgrade
branch
2 times, most recently
from
83e127f to
d1bbc6f
Compare
charliermarsh
force-pushed
the
charlie/upgrade
branch
from
d1bbc6f to
a348cf5
Compare
Summary
This PR upgrade RustPython to pull in the changes to
Arguments(zip defaults with their identifiers) and all the renames toCmpOpand friends.