-
Notifications
You must be signed in to change notification settings - Fork 43
Track position where normalization failed #114
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
base: master
Are you sure you want to change the base?
Conversation
efa20e0
to
c993dd1
Compare
Right, |
Seems like this would be a breaking change, yes? We haven't really been planning for a 0.2.0 of this crate but I guess it's possible. We can use the opportunity to increase MSRV, too, ideally to a version with the MSRV-aware resolver. |
I was mostly being conservative on MSRV since I know people get super picky about it, but yes, this would be a breaking change. I tried to not make it too breaking, which is why there are still |
If it's a breaking change anyway I think we can update MSRV to the Rust version that contains the MSRV-aware resolver. I think what we should do then is make a last release in the 0.1.0 stream and have this be the main 0.2.0 change. |
5adde99
to
436836f
Compare
Sounds reasonable to me! I'm in no rush to get this merged, so, when you need me to make changes to accommodate whatever decisions get made, I'm down to do that. For now, things seem to compile with the addition of an I believe that |
MSRV-aware resolver is 1.84, from January. The error trait is earlier. So yeah, remove that feature, and add the MSRV. https://blog.rust-lang.org/2025/01/09/Rust-1.84.0/ I'd like to have this crate have an MSRV that's roughly a year old, and now that the MSRV-aware resolver is a thing bumping the MSRV isn't as annoying to users as it used to be. |
Huh, for some reason I thought that feature was much older. I decided to update If you wanted to go with 1.85 instead, we could bump up to edition 2024, but since you mentioned only the MSRV-aware resolver, I only bumped to 2021 instead. |
out.write("#[inline]\n") | ||
out.write("pub fn is_public_assigned(c: char) -> bool {\n") | ||
out.write(" match c {\n") | ||
out.write(" matches!(\n") |
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.
This was a clippy suggestion, not required, but I figured I'd make it anyway. It's not stable in 1.36, so, that's why it wasn't being suggested before.
This basically replaces the
IsNormalized
enum in the public API with two separate types:QuickCheck
, which contains theYes
andMaybe
casesNormalizationError
, which contains theNo
case(Note:
IsNormalized::No
could have been modified to accept an error argument instead, but since theTry
trait is unstable, this would mean that?
would not be usable without a nightly-only flag for this crate.)This allows a couple things:
?
operator for normalization checks and encapsulate it in aBox<dyn Error>
if desiredTo accomodate this for the API, a few things were changed:
Chars
orCharIndices
can be left as an implementation detail. (The actual decomposition/recomposition APIs are still based upon iterators, but the check API is no longer so.)is_*
tocheck_*
, andis_*
aliases were added for the non-quick variants that still returnbool
The naming
normal_up_to
for the error was chosen for parity with the libstdUtf8Error
, which hasvalid_up_to
as its index field.I haven't actually benchmarked these changes, but I would be shocked if tracking the position substantially affected performance, especially if the result is ignored.