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

fix(compiler): ensure that placeholders have the correct sourceSpan #39717

Conversation

petebacondarwin
Copy link
Member

When the preserveWhitespaces is not true, the template parser will
process the parsed AST nodes to remove excess whitespace. Since the
generated goog.getMsg() statements rely upon the AST nodes after
this whitespace is removed, the i18n extraction must make a second pass.

Previously this resulted in innacurrate source-spans for the i18n text and
placeholder nodes that were extracted in the second pass.

This commit fixes this by reusing the source-spans from the first pass
when extracting the nodes in the second pass.

Fixes #39671

@google-cla google-cla bot added the cla: yes label Nov 16, 2020
@petebacondarwin petebacondarwin force-pushed the i18n-more-equiv-text-source-span-woes-issue-39671 branch from 06d498c to 73df49d Compare November 16, 2020 21:55
@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler area: i18n labels Nov 16, 2020
@ngbot ngbot bot modified the milestone: needsTriage Nov 16, 2020
@petebacondarwin petebacondarwin added the target: patch This PR is targeted for the next patch release label Nov 16, 2020
@petebacondarwin petebacondarwin marked this pull request as ready for review November 16, 2020 22:15
@pullapprove pullapprove bot requested a review from JoostK November 16, 2020 22:15
@petebacondarwin petebacondarwin force-pushed the i18n-more-equiv-text-source-span-woes-issue-39671 branch from dab6e08 to 08f5c7e Compare November 17, 2020 13:28
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup and the fix @petebacondarwin 👍

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Nov 17, 2020
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Nov 17, 2020
@AndrewKushnir
Copy link
Contributor

@petebacondarwin FYI, presubmit is successful for the changes in this PR. Thank you.

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 18, 2020
This helper improves the message given when an expectation
fails in a source-mapping test.
When the `preserveWhitespaces` is not true, the template parser will
process the parsed AST nodes to remove excess whitespace. Since the
generated `goog.getMsg()` statements rely upon the AST nodes after
this whitespace is removed, the i18n extraction must make a second pass.

Previously this resulted in innacurrate source-spans for the i18n text and
placeholder nodes that were extracted in the second pass.

This commit fixes this by reusing the source-spans from the first pass
when extracting the nodes in the second pass.

Fixes angular#39671
When parsing for i18n messages, interpolated strings are
split into `Text` and `Placeholder` pieces.  The method that
does this `_visitTextWithInterpolation()` was becoming too
complex. This commit refactors that method along with some
associated functions that it uses.
@petebacondarwin petebacondarwin force-pushed the i18n-more-equiv-text-source-span-woes-issue-39671 branch from 08f5c7e to 9ee0012 Compare November 19, 2020 12:25
@petebacondarwin petebacondarwin removed the request for review from JoostK November 19, 2020 22:26
@AndrewKushnir
Copy link
Contributor

Presubmit #2.

AndrewKushnir pushed a commit that referenced this pull request Nov 20, 2020
This helper improves the message given when an expectation
fails in a source-mapping test.

PR Close #39717
AndrewKushnir pushed a commit that referenced this pull request Nov 20, 2020
…39717)

When the `preserveWhitespaces` is not true, the template parser will
process the parsed AST nodes to remove excess whitespace. Since the
generated `goog.getMsg()` statements rely upon the AST nodes after
this whitespace is removed, the i18n extraction must make a second pass.

Previously this resulted in innacurrate source-spans for the i18n text and
placeholder nodes that were extracted in the second pass.

This commit fixes this by reusing the source-spans from the first pass
when extracting the nodes in the second pass.

Fixes #39671

PR Close #39717
AndrewKushnir pushed a commit that referenced this pull request Nov 20, 2020
When parsing for i18n messages, interpolated strings are
split into `Text` and `Placeholder` pieces.  The method that
does this `_visitTextWithInterpolation()` was becoming too
complex. This commit refactors that method along with some
associated functions that it uses.

PR Close #39717
AndrewKushnir pushed a commit that referenced this pull request Nov 20, 2020
…39717)

When the `preserveWhitespaces` is not true, the template parser will
process the parsed AST nodes to remove excess whitespace. Since the
generated `goog.getMsg()` statements rely upon the AST nodes after
this whitespace is removed, the i18n extraction must make a second pass.

Previously this resulted in innacurrate source-spans for the i18n text and
placeholder nodes that were extracted in the second pass.

This commit fixes this by reusing the source-spans from the first pass
when extracting the nodes in the second pass.

Fixes #39671

PR Close #39717
AndrewKushnir pushed a commit that referenced this pull request Nov 20, 2020
When parsing for i18n messages, interpolated strings are
split into `Text` and `Placeholder` pieces.  The method that
does this `_visitTextWithInterpolation()` was becoming too
complex. This commit refactors that method along with some
associated functions that it uses.

PR Close #39717
@petebacondarwin petebacondarwin deleted the i18n-more-equiv-text-source-span-woes-issue-39671 branch November 25, 2020 19:20
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler area: i18n cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ng extract-i18n extracts incorrect equiv-text value
2 participants