Skip to content
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

Add test for custom transliterator using Latin-ASCII and Lower #5469

Merged
merged 8 commits into from
Sep 6, 2024

Conversation

sffc
Copy link
Member

@sffc sffc commented Aug 30, 2024

It is a little awkward due to issues like #3991 and #3910, but I got it to work.

There are two other changes I split out into #5468 and #5467.

Depends on #5483

CC @FrankYFTang

@sffc sffc requested review from Manishearth and a team as code owners August 30, 2024 22:04
@sffc sffc requested review from robertbastian and skius and removed request for a team August 30, 2024 22:04
}

#[derive(Debug)]
struct LowercaseTransliterator(CaseMapper);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I suspect these should be exported by the transliterator crate

but we really need to figure out a better solution for bundling "default" transliterators anyway

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they should be, but I didn't feel like making big changes right now; I want to show how to do this with minimal patches against 1.5

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not proposing that for this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they should be exposed by the crate. If you need them as a dependency, you can summon them with ::Lower, otherwise just use CaseMapper.

collection.register_source(
&"und-t-und-x0-lower".parse().unwrap(),
"<error>".to_string(),
["Any-Lower"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: would love transliterator to expose all of these in some module, perhaps with individual names, as well as a bundling solution that lets you include various useful bundles of the.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trait NamedCustomTransliterator: CustomTransliterator {
    const BCP47: &'static str;
    const NAME: &'static str;
}

@sffc
Copy link
Member Author

sffc commented Sep 3, 2024

I want #5483 to land first, but this PR should still be reviewed

Manishearth
Manishearth previously approved these changes Sep 3, 2024
sffc added a commit that referenced this pull request Sep 5, 2024
…or docs test (#5483)

A more docs-friendly version of the test introduced in #5469

Related: #3991
Copy link

dpulls bot commented Sep 5, 2024

🎉 All dependencies have been resolved !

@sffc
Copy link
Member Author

sffc commented Sep 6, 2024

I reverted the "Fix transliterator fallback loading" commit since it seems like I don't actually need it.

@sffc sffc merged commit ee718ba into unicode-org:main Sep 6, 2024
28 checks passed
@sffc sffc deleted the translit-lower branch September 6, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants