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

ext/bcmath: Moved scale description from language-snippets to bcadd #4423

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

SakiTakamachi
Copy link
Member

No description provided.

@SakiTakamachi SakiTakamachi force-pushed the move_bc_scale_description branch from efcbec4 to 6df0e68 Compare January 29, 2025 05:53
@SakiTakamachi SakiTakamachi marked this pull request as ready for review January 29, 2025 06:00
@nielsdos
Copy link
Member

I'm not sure if there's much point in doing this, maybe I missed some discussion somewhere; but I won't object.

@SakiTakamachi
Copy link
Member Author

@nielsdos
Oh, sorry
the discussion is here:
#4187 (comment)

@nielsdos
Copy link
Member

I see

But I think you missed that you can giev it a special xml:id:

xml:id attribute like "bcmath-number.add..parameters.scale"

@SakiTakamachi
Copy link
Member Author

hmm

  Random removing duplicated xml:id: function.bcadd.parameters.scale

Since the includes are nested, duplicate ids seem to occur.
I don't think it's necessary to force this, so I'll close it for now.

@SakiTakamachi
Copy link
Member Author

Oh, it was my mistake to close this.
There is no need to forcefully assign an ID, so may be fine with the current PR status.

@SakiTakamachi
Copy link
Member Author

SakiTakamachi commented Jan 29, 2025

If don't assign IDs, I may not need to make this change...

edit:
However, from the translation perspective, there is an advantage to separating it from the snippet as it makes it easier to translate.

@alfsb
Copy link
Member

alfsb commented Jan 29, 2025

hmm

  Random removing duplicated xml:id: function.bcadd.parameters.scale

Since the includes are nested, duplicate ids seem to occur. I don't think it's necessary to force this, so I'll close it for now.

Per https://github.com/php/doc-base/blob/master/docs/structure.md#xmlid-structure , the xml:id would be:

function.bcadd..parameters.scale

not

function.bcadd.parameters.scale

Mind the double dot.

@SakiTakamachi
Copy link
Member Author

@alfsb
I was wrong.
Using two dots worked, I didn't understand the documentation correctly...
Thanks!

@SakiTakamachi
Copy link
Member Author

I can probably use IDs elsewhere as well, but does it seem ok to add the commit to this PR?

@SakiTakamachi
Copy link
Member Author

Leave this alone for now and create a follow-up PR for the others.

@alfsb
Copy link
Member

alfsb commented Jan 29, 2025

I'm not sure if there's much point in doing this, maybe I missed some discussion somewhere; but I won't object.

Only to clarify, this change is good because language-snippets.ent is huge, is the most frequently changed file, and it's not normal XML. All these points make this file a real PITA for translations with small teams. Avoiding adding things on it, and even better, having these "snippets" as normal XML, in normal XML files, that are infrequently changed, helps translations big time.

Also, it is way easier to debug xml:id breakages instead of DTD entity references breakages, even the nested ones.

@alfsb
Copy link
Member

alfsb commented Jan 29, 2025

I didn't understand the documentation correctly... Thanks!

I will try to change it to make more detailed.

@SakiTakamachi SakiTakamachi merged commit 3295741 into php:master Jan 29, 2025
2 checks passed
@SakiTakamachi SakiTakamachi deleted the move_bc_scale_description branch January 29, 2025 23: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