You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by audreyso <gi...@git.apache.org> on 2017/06/14 23:41:29 UTC
[GitHub] cordova-android pull request #386: CB-12895 : added eslint and removed jshin...
GitHub user audreyso opened a pull request:
https://github.com/apache/cordova-android/pull/386
CB-12895 : added eslint and removed jshint / reformatted with eslint
<!--
Please make sure the checklist boxes are all checked before submitting the PR. The checklist
is intended as a quick reference, for complete details please see our Contributor Guidelines:
http://cordova.apache.org/contribute/contribute_guidelines.html
Thanks!
-->
### Platforms affected
### What does this PR do?
Added eslint and removed jshint / reformatted with eslint.
### What testing has been done on this change?
### Checklist
- [X] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
- [X] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
- [X] Added automated test coverage as appropriate for this change.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/audreyso/cordova-android CB-12895
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-android/pull/386.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #386
----
commit 8e4e071e5d6f3427feaf6e216db9757c42c0f0ea
Author: Audrey So <au...@apache.org>
Date: 2017-06-09T18:18:57Z
CB-12895 : added eslint and removed jshint
commit 4e19a0ce08a2a117990ba20d7ef1fbb0f6ebcd5b
Author: Audrey So <au...@apache.org>
Date: 2017-06-13T18:42:20Z
CB-12895 : fixed eslint errors
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #386: CB-12895 : added eslint and removed jshint / ref...
Posted by infil00p <gi...@git.apache.org>.
Github user infil00p commented on the issue:
https://github.com/apache/cordova-android/pull/386
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #386: CB-12895 : added eslint and removed jshint / ref...
Posted by audreyso <gi...@git.apache.org>.
Github user audreyso commented on the issue:
https://github.com/apache/cordova-android/pull/386
I think I have permission, but haven't ever done it before! @filmaj
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #386: CB-12895 : added eslint and removed jshint / ref...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-android/pull/386
I believe in you, you can do it!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request #386: CB-12895 : added eslint and removed jshin...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-android/pull/386
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #386: CB-12895 : added eslint and removed jshint / ref...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-android/pull/386
I think one more tweak would be good:
So by putting `root:true` into the spec/**/ directories' eslintrc files, when eslint searches up the directory tree to find an eslintrc file, once it finds the file, it'll stop searching further. eslint, however, supports "cascading" or combining multiple config files into one. We can leverage this to define one project-level eslintrc file in the root of the project dir, but then also have additional eslintrc files in project subdirectories if there are specific rules we want to apply per-directory.
So I think what we want to do is:
- have the project-root-directory-level eslintrc file specify `root:true`
- any subdirectory-level eslintrc files _not_ to specify `root:true`
- this would then cause eslint to apply the rules from subdirectories, but keep going to search for further rules, and in our case, it would keep going until it hit the project-root-directory-level eslintrc, apply those rules, and because that one has `root:true`, stop there.
- additionally, in this repo, both the spec/e2e and spec/unit directories need just a single additional rule: `env: jasmine: true`. So we could actually put a single eslintrc file under `/spec/.eslintrc.yml` and have it contain only the one `env: jasmine: true` rule. Since root is not set to true for the `spec/.eslintrc.yml` file, it'll go up one more directory and apply the project-level eslintrc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #386: CB-12895 : added eslint and removed jshint / ref...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-android/pull/386
Ahh! That's it! Very nice. Works flawlessy on my end!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #386: CB-12895 : added eslint and removed jshint / ref...
Posted by audreyso <gi...@git.apache.org>.
Github user audreyso commented on the issue:
https://github.com/apache/cordova-android/pull/386
I rebased this morning! @filmaj @infil00p If anyone has time to merge it in, that would be great :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #386: CB-12895 : added eslint and removed jshint / ref...
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:
https://github.com/apache/cordova-android/pull/386
# [Codecov](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=h1) Report
> Merging [#386](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-android/commit/3a6e898b12eac31dfc16a4c526dfba8fab158723?src=pr&el=desc) will **increase** coverage by `2.18%`.
> The diff coverage is `51.13%`.
[](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #386 +/- ##
=========================================
+ Coverage 39.52% 41.7% +2.18%
=========================================
Files 16 16
Lines 1551 1489 -62
Branches 277 278 +1
=========================================
+ Hits 613 621 +8
+ Misses 938 868 -70
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [bin/templates/cordova/lib/builders/builders.js](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9idWlsZGVycy9idWlsZGVycy5qcw==) | `37.5% <0%> (+4.16%)` | :arrow_up: |
| [bin/templates/cordova/lib/retry.js](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9yZXRyeS5qcw==) | `15.38% <0%> (ø)` | :arrow_up: |
| [bin/templates/cordova/lib/AndroidStudio.js](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9BbmRyb2lkU3R1ZGlvLmpz) | `94.73% <100%> (ø)` | :arrow_up: |
| [bin/templates/cordova/lib/AndroidManifest.js](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9BbmRyb2lkTWFuaWZlc3QuanM=) | `35.13% <100%> (-0.87%)` | :arrow_down: |
| [...in/templates/cordova/lib/builders/GradleBuilder.js](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9idWlsZGVycy9HcmFkbGVCdWlsZGVyLmpz) | `20.43% <20.93%> (+0.43%)` | :arrow_up: |
| [bin/templates/cordova/lib/device.js](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9kZXZpY2UuanM=) | `22.44% <26.31%> (+4.26%)` | :arrow_up: |
| [bin/templates/cordova/lib/build.js](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9idWlsZC5qcw==) | `13.43% <31.57%> (+3.08%)` | :arrow_up: |
| [bin/templates/cordova/lib/Adb.js](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9BZGIuanM=) | `34.14% <33.33%> (+3.03%)` | :arrow_up: |
| [bin/templates/cordova/lib/run.js](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9ydW4uanM=) | `26.98% <37.5%> (+9.07%)` | :arrow_up: |
| [bin/templates/cordova/lib/emulator.js](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9jb3Jkb3ZhL2xpYi9lbXVsYXRvci5qcw==) | `40.81% <41.93%> (+2.47%)` | :arrow_up: |
| ... and [6 more](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=footer). Last update [3a6e898...875ea35](https://codecov.io/gh/apache/cordova-android/pull/386?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #386: CB-12895 : added eslint and removed jshint / ref...
Posted by audreyso <gi...@git.apache.org>.
Github user audreyso commented on the issue:
https://github.com/apache/cordova-android/pull/386
Thanks! @filmaj That makes sense and seems easier than having to add in /*eslint-env: jasmine*/ in every file. I made some changes and added .eslint configs where they are needed. Let me know if you have any other suggestions or feedback!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request #386: CB-12895 : added eslint and removed jshin...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/386#discussion_r124902965
--- Diff: bin/templates/cordova/Api.js ---
@@ -112,16 +110,13 @@ Api.createPlatform = function (destination, config, options, events) {
events = setupEvents(events);
var result;
try {
- result = require('../../lib/create')
- .create(destination, config, options, events)
- .then(function (destination) {
+ result = require('../../lib/create').create(destination, config, options, events).then(function (destination) {
--- End diff --
This is an unfortunate workaround for eslint-ing. I would much prefer this be broken up, ala ...
var libCreate = require('../../lib/create');
var result = libCreate.create(destination, config, options, events);
result.then(function (destination) {
// ...
});
...
return result;
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #386: CB-12895 : added eslint and removed jshint / ref...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-android/pull/386
Do you not have permissions to merge, or do you have your hands full with other work? Happy to merge this in for you but if it's a permission issue please let me know so that I can fix that :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #386: CB-12895 : added eslint and removed jshint / ref...
Posted by audreyso <gi...@git.apache.org>.
Github user audreyso commented on the issue:
https://github.com/apache/cordova-android/pull/386
Ohh thank you for that clarification @filmaj . So I did the following:
1. Left the project root level eslint file to keep--> root: true
2. Removed eslint config files from spec/e2e & spec/unit and just placed it in spec/.eslintrc.yml
3. Took out root:true from spec/.eslintrc.yml and just included ONE rule env: jasmine: true
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org