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 references to LocaleCanonicalizer #5306

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

sffc
Copy link
Member

@sffc sffc commented Jul 25, 2024

Follow-up from #5296

I couldn't linkify them because of the dependency order

@@ -27,7 +27,8 @@ use writeable::Writeable;
/// `_` separators to `-` and adjusting casing to conform to the Unicode standard.
///
/// Any bogus subtags will cause the parsing to fail with an error.
/// No subtag validation is performed.
///
/// No subtag validation or alias resolution is performed; use `LocaleCanonicalizer`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// No subtag validation or alias resolution is performed; use `LocaleCanonicalizer`.
/// For validation and canonicalization, see `LocaleCanonicalizer`.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is useful to explicitly call out that this function does not perform validation and canonicalization. If you expand the file, there is a list of three levels of conformance, and this function performs one but not the other two.

It is still a bit confusing that the previous line references "bogus subtags" without any definition of what "bogus" means.

@robertbastian robertbastian added the waiting-on-author PRs waiting for action from the author for >7 days label Aug 13, 2024
@sffc
Copy link
Member Author

sffc commented Aug 15, 2024

@zbraniecki Do you have a wording suggestion?

@sffc sffc added needs-approval One or more stakeholders need to approve proposal and removed waiting-on-author PRs waiting for action from the author for >7 days labels Aug 15, 2024
@sffc sffc requested a review from zbraniecki August 15, 2024 01:05
@zbraniecki
Copy link
Member

I don't like bogus here - it seems underdefined.
Suggestion:

/// Any syntactically invalid subtags will cause the parsing to fail with an error.
/// This operation normalizes syntax only. No legacy subtag replacements is performed.
/// For data validation and subtag canonicalization, see `LocaleCanonicalizer`.

@sffc
Copy link
Member Author

sffc commented Aug 22, 2024

OK I applied that suggestion. Look good to merge?

zbraniecki
zbraniecki previously approved these changes Aug 22, 2024
/// Any syntactically invalid subtags will cause the parsing to fail with an error.
///
/// This operation normalizes syntax only. No legacy subtag replacements is performed.
/// For data validation and subtag canonicalization, see `LocaleCanonicalizer`.
Copy link
Member

Choose a reason for hiding this comment

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

Why "data validation" instead of "validation"? Why "subtag canonicalization" instead of "canonicalization? The list above doesn't say "well-formed", "data-valid", and "subtag-canonical".

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with changing it to:

This operation normalizes syntax to well-formed. No legacy subtag replacements is performed.
For validation and canonicalization, see LocaleCanonicalizer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

nb: I would even shorten it to

This operation normalizes syntax to be well-formed. For validation and canonicalization, see LocaleCanonicalizer.

because it follows from the definition above that "normalizing syntax to be well-formed" does just that, and subtag replacement is part of canonicalization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-approval One or more stakeholders need to approve proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants