Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
clarfonthey wants to merge1 commit intounicode-rs:master
base:master
Choose a base branch
Loading
fromclarfonthey:errors

Conversation

@clarfonthey
Copy link

@clarfontheyclarfonthey commentedOct 8, 2025
edited
Loading

This basically replaces theIsNormalized enum in the public API with two separate types:

  1. QuickCheck, which contains theYes andMaybe cases
  2. NormalizationError, which contains theNo case

(Note:IsNormalized::Nocould 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:

  • If the lack of normalization is an error (e.g. a configuration format requires it), the position of the error can be pointed out to a user
  • Another error type can point to the normalization error as an underlying cause (if, for example, the error is "invalid string", this could explain why)
  • You can use the? operator for normalization checks and encapsulate it in aBox<dyn Error> if desired

To accomodate this for the API, a few things were changed:

  • Quick-check functions don't take iterators, just strings. This way, the fact that they take eitherChars 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.)
  • The methods were renamed fromis_* tocheck_*, andis_* aliases were added for the non-quick variants that still returnbool

The namingnormal_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.

@clarfontheyclarfontheyforce-pushed theerrors branch 2 times, most recently fromefa20e0 toc993dd1CompareOctober 8, 2025 05:14
@clarfonthey
Copy link
Author

Right,core::error is a recent addition, so, it'll need a feature flag. I can add that in with some shenanigans so it always implementsError withstd, but requires the extra feature if it's disabled.

@Manishearth
Copy link
Member

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.

clarfonthey reacted with thumbs up emoji

@clarfonthey
Copy link
Author

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 ittoo breaking, which is why there are stillis_* aliases, but it is very strictly breaking by changing return types.

@Manishearth
Copy link
Member

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.

clarfonthey reacted with thumbs up emoji

@clarfontheyclarfontheyforce-pushed theerrors branch 2 times, most recently from5adde99 to436836fCompareOctober 8, 2025 17:33
@clarfonthey
Copy link
Author

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 anerror feature that only implements theError trait when that feature is enabled. With 1.36, that means it won't work if you have the specific combination of--no-default-features --features error, but every other combination works.

I believe thatcore::error was stabilised after the MSRV-dependent resolver, so, we might still want this, but if not, I can remove the feature and just make it always implement the trait.

@Manishearth
Copy link
Member

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 reacted with thumbs up emoji

@clarfonthey
Copy link
Author

clarfonthey commentedOct 9, 2025
edited
Loading

Huh, for some reason I thought that feature was much older.

I decided to updaterust-version andedition for this crate too to see what it would affect, and everything seems okay. Feel free to copy those changes to another PR if you'd prefer. I also set the crate version to 0.2.0 since you seemed keen on making this the main 0.2.0 change.

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")
Copy link
Author

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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ManishearthManishearthManishearth left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@clarfonthey@Manishearth

[8]ページ先頭

©2009-2025 Movatter.jp