- Notifications
You must be signed in to change notification settings - Fork44
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
efa20e0 toc993dd1Compareclarfonthey commentedOct 8, 2025
Right, |
Manishearth commentedOct 8, 2025
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. |
Uh oh!
There was an error while loading.Please reload this page.
clarfonthey commentedOct 8, 2025
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 ittoo breaking, which is why there are still |
Manishearth commentedOct 8, 2025
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 to436836fCompareclarfonthey commentedOct 9, 2025
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 |
Manishearth commentedOct 9, 2025
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. |
clarfonthey commentedOct 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Uh oh!
There was an error while loading.Please reload this page.
This basically replaces the
IsNormalizedenum in the public API with two separate types:QuickCheck, which contains theYesandMaybecasesNormalizationError, which contains theNocase(Note:
IsNormalized::Nocould have been modified to accept an error argument instead, but since theTrytrait 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:
CharsorCharIndicescan 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 returnboolThe naming
normal_up_tofor the error was chosen for parity with the libstdUtf8Error, which hasvalid_up_toas 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.