refactor(compiler): trim expression whitespace when preserveSignificantWhitespace === false#58176
Closed
dgp1130 wants to merge 3 commits intoangular:mainfrom
Closed
refactor(compiler): trim expression whitespace when preserveSignificantWhitespace === false#58176dgp1130 wants to merge 3 commits intoangular:mainfrom
preserveSignificantWhitespace === false#58176dgp1130 wants to merge 3 commits intoangular:mainfrom
Conversation
This serializes the expression AST back into a string. This is useful to normalize whitespace in expressions so i18n messages are not affected by insignificant changes (such as going from `{{ foo }}` to `{{\n foo\n}}`).
… `preserveSignificantWhitespace === false`. This parses and reserializes expressions to normalize their whitespace formatting and make them more durable to insignificant changes in whitespace which might otherwise alter message IDs despite no translator-meaningful change being made.
While effective, `preservePlaceholders` unfortunately is not viable in google3 at the moment due to some complexities with how TC extracts messages. Therefore this feature is being removed in favor of whitespace trimming of expressions, which is viable for TC and provides most of the same benefit. This is a partial revert of dab722f.
AndrewKushnir
approved these changes
Oct 12, 2024
Contributor
AndrewKushnir
left a comment
There was a problem hiding this comment.
Looks great, thanks @dgp1130 👍
Contributor
Author
|
TGP (with flag disabled): http://test/OCL:685031311:BASE:685031062:1728703209245:53d763a5 I'll run another TGP with the flag enabled just to make sure enabling it won't cause any problems. |
Contributor
Author
|
TGP with the relevant flag enabled. There are some XMB golden tests which failed, but that's expected because enabling the flag changes message IDs, so there's no issue. One project had a TC extraction failure due to a couple of now-duplicate messages, but we can avoid that by just adding a Should be good to merge! |
Member
|
This PR was merged into the repository by commit 6d33ab5. The changes were merged into the following branches: main |
devversion
pushed a commit
that referenced
this pull request
Oct 16, 2024
… `preserveSignificantWhitespace === false`. (#58176) This parses and reserializes expressions to normalize their whitespace formatting and make them more durable to insignificant changes in whitespace which might otherwise alter message IDs despite no translator-meaningful change being made. PR Close #58176
devversion
pushed a commit
that referenced
this pull request
Oct 16, 2024
While effective, `preservePlaceholders` unfortunately is not viable in google3 at the moment due to some complexities with how TC extracts messages. Therefore this feature is being removed in favor of whitespace trimming of expressions, which is viable for TC and provides most of the same benefit. This is a partial revert of dab722f. PR Close #58176
Closed
crisbeto
added a commit
to crisbeto/angular
that referenced
this pull request
Oct 16, 2024
Fixes some tests that started failing, because angular#58154 made it so placeholder-only messages are extracted while angular#58176 added some tests that only contain placeholders.
devversion
pushed a commit
that referenced
this pull request
Oct 16, 2024
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR trims whitespace in expressions when
preserveSignificantWhitespace === false. This allows messages to be a bit more durable to insignificant whitespace changes. For example, reformatting the following:Into:
Is a whitespace-only change which has no visible effect to translators. Therefore we trim whitespace before extracting messages and normalize it to a standard format (
{{name}}).This PR also removes the functionality for ignoring placeholder expressions entirely (originally added as part of dab722f). Ideally, we wouldn't take placeholders into account at all, since whether an expression is
{{foo}}or{{bar}}is also irrelevant to the translator. However doing so causes some challenges with TC, so we can't go that far just yet (see http://b/367255149#comment10). Therefore this PR rolls back that particular feature and replaces it with this one which trims expression whitespace instead.