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.

Sign up for GitHub

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

Jump to bottom

test(compiler): allow asserting matching identifier names #22835

Closed
wants to merge 2 commits into from
Closed

test(compiler): allow asserting matching identifier names #22835

wants to merge 2 commits into from

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Mar 17, 2018

We will need to assert identifier names for i18n (/MSG_[A-Z_]+/)

@vicb vicb added feature Issue that requests a new feature action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: major This PR is targeted for the next major release and removed cla: yes labels Mar 17, 2018
@mary-poppins
Copy link

You can preview 7acde39 at https://pr22835-7acde39.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d51c1ff at https://pr22835-d51c1ff.ngbuilds.io/.

@mary-poppins
Copy link

You can preview c302143 at https://pr22835-c302143.ngbuilds.io/.

ocombe
ocombe reviewed Mar 19, 2018
View reviewed changes
packages/compiler/test/render3/mock_compile.ts Outdated

if (pieces.length === 0 || TOKEN.lastIndex !== 0) {
const from = TOKEN.lastIndex;
const to = from + 30;
Copy link
Contributor

Choose a reason for hiding this comment

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

why "+30"? use a const for magic numbers ;)

packages/compiler/test/render3/mock_compile.ts Outdated
}
}

const IDENT_LIKE = /^[a-z][A-Z]/;
const SPECIAL_RE_CHAR = /\/|\(|\)|\||\*|\+|\[|\]|\{|\}|\$/g;
function r(...pieces: (string | RegExp)[]): RegExp {
function r(pieces: (string | RegExp)[]): {regexp: RegExp, groups: Map<string, number>} {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is r doing? Any reason for a single letter name? Or maybe add a comment that explains what the function does

@mary-poppins
Copy link

You can preview 88db204 at https://pr22835-88db204.ngbuilds.io/.

@vicb vicb 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 Mar 20, 2018
@ngbot
Copy link

ngbot bot commented Mar 20, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "continuous-integration/travis-ci/pr" is failing
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@vicb vicb added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Mar 20, 2018
@vicb
Copy link
Contributor Author

vicb commented Mar 20, 2018

Need to be rebased once master is green (the failures are not related to the changes in this PR).

@vicb
Copy link
Contributor Author

vicb commented Mar 20, 2018

http://test/OCL:189801199:BASE:189801247:1521578595666:988ec562

@mary-poppins
Copy link

You can preview dc3af82 at https://pr22835-dc3af82.ngbuilds.io/.

@IgorMinar
Copy link
Contributor

master is green but affected by sauce-labs flakes :-(
I'm rerunning the tests now

@vicb vicb removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Mar 21, 2018
@mary-poppins
Copy link

You can preview 092f142 at https://pr22835-092f142.ngbuilds.io/.

@matsko matsko closed this in 129bb18 Mar 23, 2018
matsko pushed a commit that referenced this pull request Mar 23, 2018
@vicb @matsko
test(compiler): allow asserting matching identifier names (#22835)
5ab9d4d 
PR Close #22835
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@vicb @leo6104
refactor(compiler): refactor code matching helpers for compliance spec (
8463c78 
angular#22835)

PR Close angular#22835
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@vicb @leo6104
test(compiler): allow asserting matching identifier names (angular#22835
983bf21 
)

PR Close angular#22835
@vicb vicb deleted the 0316-mockCompiler branch April 3, 2018 17:59
@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 Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ocombe ocombe ocombe left review comments

@chuckjaz chuckjaz chuckjaz approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes feature Issue that requests a new feature target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@vicb @mary-poppins @IgorMinar @ocombe @chuckjaz @googlebot