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

@petebacondarwin petebacondarwin commented Nov 16, 2020

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 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 Nov 16, 2020
@pullapprove pullapprove bot requested a review from JoostK Nov 16, 2020
@petebacondarwin petebacondarwin force-pushed the i18n-more-equiv-text-source-span-woes-issue-39671 branch from dab6e08 to 08f5c7e Compare Nov 17, 2020
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Thanks for the cleanup and the fix @petebacondarwin 👍

@AndrewKushnir AndrewKushnir added the action: presubmit A standard presubmit is running / required label Nov 17, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Nov 17, 2020

Presubmit.

@AndrewKushnir AndrewKushnir removed the action: presubmit A standard presubmit is running / required label Nov 17, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Nov 17, 2020

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

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 Nov 19, 2020
@petebacondarwin petebacondarwin removed the request for review from JoostK Nov 19, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Nov 19, 2020

Presubmit #2.

AndrewKushnir pushed a commit that referenced this issue 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 issue 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 issue 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 issue 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 issue 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 Nov 25, 2020
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Dec 26, 2020

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 PR author is ready for this to merge cla: yes comp: compiler comp: i18n target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants