Skip to content
This repository has been archived by the owner on Nov 6, 2023. It is now read-only.

Add ESLint Travis task #10897

Merged
merged 13 commits into from Jul 7, 2017
Merged

Add ESLint Travis task #10897

merged 13 commits into from Jul 7, 2017

Conversation

ghost
Copy link

@ghost ghost commented Jul 6, 2017

Note that this doesn't include any ESLint rules and ESLint will always succeed until we'll add the rules. I will create 3 more PRs with different ESLint rules: Standard, Airbnb and Google.

@ghost
Copy link
Author

ghost commented Jul 6, 2017

ping @Hainish

This was referenced Jul 6, 2017
@Hainish
Copy link
Member

Hainish commented Jul 6, 2017

We've generally put tools like this in utils/

@ghost
Copy link
Author

ghost commented Jul 6, 2017

@Hainish It is in /test/.

@ghost
Copy link
Author

ghost commented Jul 6, 2017

@Hainish Or do you want ESLint to be installed under /utils/? Usually dev dependencies are installed under node-modules, just like regular dependencies.

@ghost
Copy link
Author

ghost commented Jul 6, 2017

@Hainish Yeah, package.json is supposed to be in /chromium/. It's not an utility.

@ghost
Copy link
Author

ghost commented Jul 6, 2017

@ghost
Copy link
Author

ghost commented Jul 6, 2017

@Hainish Also, node_modules, .gitignore, .eslintrc.json and package.json should be excluded from the crx, just in case.

test/travis.sh Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jul 6, 2017

@Hainish How can I exclude a file or a directory from the crx?

@Hainish
Copy link
Member

Hainish commented Jul 6, 2017

This is a utility run over the chromium/ codebase. If there are other utilities added in the future, they would also need to be in their own separate paths in utils/. This is necessary to keep the repo tidy, not just throwing a package.json in the chromium/ codebase for this particular use case.

@ghost
Copy link
Author

ghost commented Jul 6, 2017

@Hainish We may need package.json later, when we will be adding unit tests ran in Node environment using a test runner like Mocha, but OK, I will remove it for now.

@ghost
Copy link
Author

ghost commented Jul 6, 2017

@Hainish Done.

@ghost
Copy link
Author

ghost commented Jul 6, 2017

@Hainish Ready for merge.

@Hainish
Copy link
Member

Hainish commented Jul 6, 2017

@koops76 for some reason, the eslnit run isn't working as intended. Even with a junk commit in #10899, the eslint test passes: https://travis-ci.org/EFForg/https-everywhere/jobs/250977450

@ghost
Copy link
Author

ghost commented Jul 7, 2017

@Hainish Of course. There are no rules set up to lint against.

@Hainish
Copy link
Member

Hainish commented Jul 7, 2017

@koops76 this was properly failing previously, before moving to utils/

@ghost
Copy link
Author

ghost commented Jul 7, 2017

@Hainish Try now.

koops76 added 2 commits July 7, 2017 03:55
This reverts commit 886b975.
@ghost
Copy link
Author

ghost commented Jul 7, 2017

@Hainish It works.
https://travis-ci.org/EFForg/https-everywhere/jobs/250990887
https://travis-ci.org/EFForg/https-everywhere/jobs/250991973

@Hainish Hainish merged commit dba80b3 into EFForg:master Jul 7, 2017
@ghost ghost deleted the eslint branch July 7, 2017 01:09
@ghost ghost restored the eslint branch July 16, 2017 16:47
@ghost ghost marked this as a duplicate of #9347 Jul 18, 2017
@ghost ghost mentioned this pull request Jul 18, 2017
@ghost ghost deleted the eslint branch August 9, 2017 16:07
luciancor pushed a commit to luciancor/https-everywhere that referenced this pull request Aug 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant