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

[format.string.std] Replace "Derived Extracted Property" with simply "property" #6224

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

cpplearner
Copy link
Contributor

"Derived Extracted Property" refers to the fact that the data for the East_Asian_Width property are extracted into the file DerivedEastAsianWidth.txt. It is not really a terminology.

@jensmaurer
Copy link
Member

@cor3ntin , @tahonermann , any opinions on the above? The rationale sounds plausible.

@cor3ntin
Copy link
Contributor

Unicode says

When referencing a Unicode character property, it is customary to prepend the word “Unicode” to the name of the property, unless it is clear from context that the Unicode Standard
is the source of the specification.

I think the change make sense but maybe we should do something similar for http://eel.is/c++draft/lex.name#1

@tahonermann
Copy link
Contributor

I agree that the change is reasonable; the context is sufficient for readers to discover details about the property as desired. I also agree with Corentin's suggestion to replace similar wording elsewhere.

@jensmaurer
Copy link
Member

jensmaurer commented Apr 13, 2023

@cor3ntin , what do you want to change in http://eel.is/c++draft/lex.name#1 ? The "Derived Core Properties" mention is in an note, explaining XID_Start and XID_Continue. That seems reasonably explanatory to me.

@cor3ntin
Copy link
Contributor

Maybe something like
The properties XID_Start and XID_Continue are described by UAX #44 of the Unicode Standard.

Ie the fact that they are derived is not really adding any information

@tkoeppe
Copy link
Contributor

tkoeppe commented Jul 31, 2024

@cpplearner Could you please have another look?

@cpplearner
Copy link
Contributor Author

I agree that the mention of "Derived Core Properties" is unneeded. But it's slightly different from the case with East_Asian_Width, which is not a derived property, and it's wrong to describe it as such.

@tahonermann
Copy link
Contributor

@cpplearner, can you elaborate on that concern? Unless I'm missing something, the proposed change simply refers to East_Asian_Width as a "property". That looks fine to me.

@tkoeppe, can you also make the change that @cor3ntin suggested?

@tkoeppe
Copy link
Contributor

tkoeppe commented Jul 31, 2024

@tahonermann Do you mean as a separate, unrelated change?

@tahonermann
Copy link
Contributor

@tahonermann Do you mean as a separate, unrelated change?

@tkoeppe, preferably as part of this change since it is somewhat related.

@tkoeppe
Copy link
Contributor

tkoeppe commented Jul 31, 2024

@tahonermann Ah, OK, but it's not my pull request :-) We'll have to ask @cpplearner.

@cpplearner
Copy link
Contributor Author

I'm unable to make the change due to network issues, and since this PR has "allow edits by maintainers" checked, maintainers can push to my branch as they wish :)

@cpplearner
Copy link
Contributor Author

I finally managed to commit the change.

Ping

Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

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

Works for me.

@cor3ntin
Copy link
Contributor

cor3ntin commented Aug 9, 2024

LGTM

@tahonermann
Copy link
Contributor

Ship it!

@wg21bot wg21bot added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Oct 16, 2024
@cpplearner
Copy link
Contributor Author

rebased.

ping @tkoeppe

@jensmaurer jensmaurer removed the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Nov 2, 2024
Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

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

Let's do that.

@jensmaurer jensmaurer merged commit aa53618 into cplusplus:main Nov 4, 2024
2 checks passed
@cpplearner cpplearner deleted the patch-1 branch November 4, 2024 07:59
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.

7 participants