Use structured types for parsing and formatting language and ABI tags #10525
Conversation
charliermarsh
added
the
internal
charliermarsh
force-pushed
the
charlie/tags
branch
3 times, most recently
from
1d3ab2a to
4b6aaba
Compare
crates/uv-platform-tags/src/abi.rs
Outdated
|
|
||
| /// Parse an [`AbiTag`] from a string. | ||
| #[allow(clippy::cast_possible_truncation)] | ||
| fn from_str(s: &str) -> Result<Self, Self::Err> { |
There was a problem hiding this comment.
This parsing code is incredibly verbose... It's fairly rote though, and comes with tests. Open to input on how to trim it down. I could perhaps remove all the error-handling...? And just return None?
charliermarsh
force-pushed
the
charlie/tags
branch
4 times, most recently
from
142cc14 to
0365fae
Compare
charliermarsh
force-pushed
the
charlie/tags
branch
6 times, most recently
from
5da08b0 to
9bfe1b7
Compare
| tag: full_tag.to_string(), | ||
| })?; | ||
| Ok((major, minor)) | ||
| } |
There was a problem hiding this comment.
I think my comments from above apply here too.
I personally am a fan of very granular error reporting like you have here. I've just been bitten so many times by overly generic and unhelpful error messages that I like this granular style much more. Like, it's more code, but I don't see this changing much or otherwise being hard to maintain personally.
| Abi3, | ||
| /// Ex) `cp39m`, `cp310t` | ||
| CPython { | ||
| gil_disabled: bool, |
There was a problem hiding this comment.
Just wondering, why this naming? We use "freethreaded" elsewhere and that avoids a double-negative (e.g., gil disabled = false is slightly harder to reason about)
There was a problem hiding this comment.
Dear goodness it comes from Py_GIL_DISABLED and is present elsewhere in our code. I defer to you on what's best here, but have a preference for "freethreaded"
There was a problem hiding this comment.
I'll just leave it for now and revisit in a separate PR.
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error, PartialEq, Eq)] | ||
| pub enum ParseAbiTagError { |
There was a problem hiding this comment.
You could split the error into the input value and the error kind with context, which should cut down the error handling boilerplate in AbiTag::from_str:
#[error("{kind} ABI tag: {tag}")]
struct ParseAbiTagError {
tag: String,
kind: ParseAbiTagErrorKind,
}
enum ParseAbiTagErrorKind {
#[error("Unknown format for")]
UnknownFormat,
#[error("Missing major version in {0}")]
MissingMajorVersion(&'static str),
// ...
}
charliermarsh
force-pushed
the
charlie/tags
branch
from
9bfe1b7 to
a986bf8
Compare
charliermarsh
force-pushed
the
charlie/tags
branch
from
a986bf8 to
f1115d0
Compare
## Summary This PR extends the thinking in #10525 to platform tags, and then uses the structured tag enums everywhere, rather than passing around strings. I think this is a big improvement! It means we're no longer doing ad hoc tag parsing all over the place.
Summary
I need to be able to do non-lexicographic comparisons between tags (e.g., so I can sort
cp313as greater thancp39). It ended up being easiest to just create structured types for all the tags we support, withFromStrandDisplayimplementations.We don't currently store these in
Tagsor inWheelFilename. We may want to, since they're really small (andCopy), but I need to benchmark to determine whether parsing these inWheelFilenameis prohibitively slow.