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

update: forbidden identifiers check all source files #968

Merged
merged 4 commits into from
Aug 13, 2016

Conversation

devversion
Copy link
Member

  • Removes invalid relative import from other scope.
  • Forbidden identifiers should run against all sources when not checking a PR.
  • Cleanup Forbidden identifiers script, by using ES6 arrow functions.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 9, 2016
@jelbourn
Copy link
Member

jelbourn commented Aug 9, 2016

@devversion FYI I fixed that import in my other PR #961 (it was failing travis)

@devversion
Copy link
Member Author

@jelbourn That works too. This PR is mainly to change the behavior of the forbidden identifiers check.

I had a quick discussion with @hansl regarding running the forbidden identifiers check against all source files when not validating a Pull Request.

I will rebase my PR once your fix is in.

@devversion devversion changed the title fix(button): remove relative import from other scope. update: forbidden identifiers check all source files Aug 12, 2016
* Resolves all files, which should run against the forbidden identifiers check.
* @return {Promise.<Array.<string>>} Files to be checked.
*/
function resolveFiles() {
Copy link
Member

Choose a reason for hiding this comment

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

The name of this function doesn't do a good job of describing what it does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't think of anything better - I'll try to find something ;)

* Removes invalid relative import from other scope.
* Forbidden identifiers should run against all sources when not checking a PR.
* Cleanup Forbidden identifiers script, by using ES6 arrow functions.

Fixes angular#967.

/* Only match .js or .ts, and remove .d.ts files. */
.then(fileList => {
return fileList.filter(name => name.match(/\.(js|ts)$/) && !name.match(/\.d\.ts$/));
Copy link
Member

@jelbourn jelbourn Aug 13, 2016

Choose a reason for hiding this comment

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

You can use .search() instead of .match() whenever you're just checking for the presence of a match and don't need the match itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

The most of those changes weren't actually from me. Also it seems like .search is not super elegant, because it returns -1, which can't be easily tested with a falsy check.

I recommend regex.test() which returns a plain boolean.

@devversion devversion force-pushed the fix/button-relative-scope-import branch from bbb416e to 7ef4919 Compare August 13, 2016 20:45
@jelbourn
Copy link
Member

lgtm

@jelbourn jelbourn merged commit 5160f58 into angular:master Aug 13, 2016
jelbourn pushed a commit to jelbourn/components that referenced this pull request Aug 14, 2016
* fix(button): remove relative import from other scope.

* Removes invalid relative import from other scope.
* Forbidden identifiers should run against all sources when not checking a PR.
* Cleanup Forbidden identifiers script, by using ES6 arrow functions.

Fixes angular#967.
@devversion devversion deleted the fix/button-relative-scope-import branch November 4, 2016 16:29
@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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants