Upgrade Eslint from 3.18.0 to 4.19.0 #4820

apb7 · 2018-03-18T06:49:58Z

Explanation

This PR upgrades Eslint to the latest version so that new rules available in the 4.X versions can be utilized. The major change occurs in the indentation. All indentation errors have been fixed.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
The PR explanation includes the words "Fixes #bugnum: ...".
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

Update Fork

seanlip

Thanks!

apb7 · 2018-03-18T06:57:26Z

Hi @seanlip! Since the indent rule became more strict, a number of files had to be fixed although all indentation parameters are still the same. We can continue to use the 3.X indent rule by indent-legacy but the indent-legacy rule would be deprecated and will not receive bugfixes or improvements in the future, so we would have to eventually switch back to indent.
Also, the official documentation states:

We recommend upgrading without changing your indent configuration, and fixing any new indentation errors that appear in your codebase.

Therefore I have fixed all such indent errors occurring with the new indent rule.
Thanks!

apb7 · 2018-03-18T08:43:55Z

The Travis log says:
npm ERR! peer dep missing: ajv@^5.0.0, required by ajv-keywords@2.1.1

This is an npm problem. I found a number of issues related to this: npm Issue #19877 and eslint PR #10022. I am looking into the solution.
Thanks!

apb7 · 2018-03-18T09:08:59Z

@seanlip: I found out that eslint-plugin-html is causing this problem. I had also bumped up its version to 4.0.1. Since we do not use the html plugin (There is no option pertaining to the html plugin in .eslintrc), I think it is safe to remove it. Please suggest.
Thanks!

apb7 · 2018-03-18T09:25:15Z

So, I found out that eslint-plugin-html was added in PR #3264 and was authored by @kevinlee12.
Hi @kevinlee12! Do we require the html plugin now? I could not locate its settings in .eslintrc.
Thanks!

codecov-io · 2018-03-18T09:46:26Z

Codecov Report

Merging #4820 into develop will increase coverage by 0.03%.
The diff coverage is 41.58%.

@@             Coverage Diff             @@
##           develop    #4820      +/-   ##
===========================================
+ Coverage    44.71%   44.74%   +0.03%     
===========================================
  Files          385      387       +2     
  Lines        23478    23515      +37     
  Branches      3794     3799       +5     
===========================================
+ Hits         10498    10522      +24     
- Misses       12980    12993      +13

Impacted Files	Coverage Δ
.../pages/exploration_player/AudioPreloaderService.js	`85.71% <ø> (ø)`	⬆️
...r/settings_tab/CollectionDetailsEditorDirective.js	`2.22% <ø> (ø)`	⬆️
...head/pages/exploration_player/AudioBarDirective.js	`0.8% <ø> (ø)`	⬆️
...v/head/pages/exploration_player/LearnerLocalNav.js	`3.84% <ø> (ø)`	⬆️
...sions/interactions/LogicProof/static/js/student.js	`89.36% <ø> (ø)`	⬆️
...ma_editors/SchemaBasedExpressionEditorDirective.js	`33.33% <ø> (ø)`	⬆️
...n_editor/editor_tab/CollectionLinearizerService.js	`99.06% <ø> (ø)`	⬆️
...sions/interactions/CodeRepl/directives/CodeRepl.js	`34.78% <ø> (ø)`	⬆️
...editor/editor_tab/CollectionNodeEditorDirective.js	`5.88% <ø> (ø)`	⬆️
...e/templates/dev/head/services/ValidatorsService.js	`90.9% <ø> (ø)`	⬆️
... and 84 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b1df68...e33f064. Read the comment docs.

kevinlee12 · 2018-03-18T19:46:36Z

Hi @apb7, I believe this was done for future implementation of this feature. It's using the default rules and is activated in .eslintrc.

oppia/.eslintrc

Lines 5 to 8 in 3b1df68

    
           "plugins": [ 
        
             "html", 
        
             "angular" 
        
           ],

If you are having trouble, did the migration guide help?

apb7 · 2018-03-19T02:58:46Z

Hi @kevinlee12! I did follow the migration guide. The problem is one of the dependencies, ajv is obsolete and therefore the html plugin does not install. The Travis log was:.
npm ERR! peer dep missing: ajv@^5.0.0, required by ajv-keywords@2.1.1

apb7 · 2018-03-19T09:25:39Z

Hi @seanlip and @kevinlee12! We need to update ajv and ajv-keywords in the Travis cache. Is it possible to clear the cache for these two node modules only so that they automatically get installed when the HTML plugin installs?
Thanks!

seanlip · 2018-03-19T09:46:05Z

I have no objection. We can delete the Travis cache whenever you like.

…

apb7 · 2018-03-19T09:49:07Z

Okay, got it! I will try clearing the cache once.
Thanks!

apb7 · 2018-03-19T10:17:27Z

@seanlip: I found a way around it. Instead of clearing cache, I have added the required dependencies to the Travis environment. OnceTravis caches these dependencies, I will remove these statements.
I think this approach should work.
Thanks!

apb7 · 2018-03-19T20:16:13Z

Hello @seanlip! Is this good to merge now?
Thanks!

seanlip

Hi @apb7 -- I just took a quick look and noticed a few more things. Comments inline! Sorry for not bringing them up earlier.

seanlip · 2018-03-19T20:20:37Z

core/templates/dev/head/components/CollectionCreationService.js

@@ -54,7 +54,7 @@ oppia.factory('CollectionCreationService', [
          }, function() {
            $rootScope.loadingMessage = '';
          }
-        );
+          );


This seems weird. Why is it indented at this level?

EDIT: Ah, it's because that's where the left paren opens. In this case move this to the end of the previous line.

Okay, got it!

seanlip · 2018-03-19T20:21:21Z

core/templates/dev/head/components/RatingDisplayDirective.js

-              ratingValue < $scope.stars[i].value - 0.75 ? 'fa-star-o' :
-              ratingValue < $scope.stars[i].value - 0.25 ? 'fa-star-half-o' :
-              'fa-star');
+                ratingValue < $scope.stars[i].value - 0.75 ? 'fa-star-o' :


Is there a way to force indent to happen only after "(", "[" and "{", but not be triggered by ":" or "?" or "&&", or ...?

Looking into this!

I did look into this. Eslint does not provide any such option to force indent.
Thanks!

Sorry -- what I meant was more along the lines of, preventing additional indenting after ":", "?", etc. I'm not sure it makes sense for those things to start a "new scope", as brackets do.

Did this behaviour change recently with eslint? Is it worth writing a custom rule?

I found out that there exists an option for ternary expressions, flatTernaryExpresions which is false by default. I have set it to true now. This would fix this problem.

seanlip · 2018-03-19T20:22:59Z

core/templates/dev/head/components/StateGraphLayoutService.js

@@ -148,7 +148,7 @@ oppia.factory('StateGraphLayoutService', [
            if (numBacktrackingCalls <= MAX_BACKTRACKING_CALLS) {
              for (var i = 0; i < adjacencyLists[currentNodeId].length; i++) {
                if (currentPath.indexOf(
-                      adjacencyLists[currentNodeId][i]) === -1) {
+                  adjacencyLists[currentNodeId][i]) === -1) {


Would be nice to have custom rule that indents this additionally by 2 (since the if clause spans a continuation line), similar to Python. OK to do this in a separate pass.

Sure, I have added it to my draft :)

@seanlip: I tried this out but the present indent rule raises an error if we indent L151 further by 2 spaces. Since consistency is still maintained throughout the codebase, we can adopt these indent settings. How does this sound?
Thanks!

If there isn't a way to enforce the "additional indent by 2" (similar to https://legacy.python.org/dev/peps/pep-0008/#indentation for python) then I'm fine with the new convention.

Eslint offers indent-legacy rule but at the same time, their official documentation states:

The indent-legacy rule is deprecated and will not receive bugfixes or improvements in the future, so you should eventually switch back to the indent rule.
To address: We recommend upgrading without changing your indent configuration, and fixing any new indentation errors that appear in your codebase.

Therefore I think it would be better if we adopted the new convention.
Thanks!

OK, happy to defer to your call. Thanks for the explanation!

Great! Let's go ahead with the new convention then. Thanks :)

seanlip · 2018-03-19T20:25:06Z

core/templates/dev/head/domain/exploration/OutcomeObjectFactorySpec.js

+    expect(testOutcome3.isConfusing(currentState)).toBe(false);
+    expect(testOutcome4.isConfusing(currentState)).toBe(true);
+    expect(testOutcome5.isConfusing(currentState)).toBe(true);
+  }


See above - can we avoid having } come above ) with the same level of indentation? Looks weird. If this is needed, move ) to the end of the previous line instead.

I am presently moving ) to the previous line. I'll look for a solution for it.

seanlip · 2018-03-19T20:25:23Z

core/templates/dev/head/domain/exploration/OutcomeObjectFactorySpec.js

@@ -25,19 +25,19 @@ describe('Outcome object factory', function() {

  it('should correctly determine if an outcome is confusing given a ' +


Probably should break after '(' since the contents of the paren span more than one line?

We currently do not have any rule implemented for this. I am working on a custom rule to take care of this. I will make a separate PR for it. :)

In the meantime shall we break it? Odd for the function() two lines below to have the same indentation level as it().

Yes, that sounds greate! I will take care of it.

seanlip · 2018-03-19T20:25:47Z

extensions/interactions/GraphInput/directives/GraphInput.js

-            text: 'Weighted',
-            option: 'isWeighted'
-          }];
+                                 {


Woah. What happened here?

This is very weird! Looking into such indentations (I found a few above too). I am not too sure why this is occurring unless it is a false positive.

@seanlip: I tried out various 4.X versions, namely 4.7.0, 4.18.0, 4.18.1, 4.18.2 but the indent rule seems to produce the same result. The indent-legacy rule works perfectly fine though. What should we do here then?
Thanks!

I have shifted { to the next line. This fixes the indentation (as suggested in Issue #10106 by the Eslint team).

I read their response differently. I think you're missing some kind of config option. The following should be valid and we need to find a way to make it so.

$scope.graphOptions = [{ text: 'Labeled', option: 'isLabeled' }, { text: 'Directed', option: 'isDirected' }, { text: 'Weighted', option: 'isWeighted' }];

Also, the following should be valid (and is preferable due to compactness):

$scope.graphOptions = [{ text: 'Labeled', option: 'isLabeled' }, { text: 'Directed', option: 'isDirected' }, { text: 'Weighted', option: 'isWeighted' }];

So some linter changes are needed in that regard.

Yes, you're right. There must be some way to get around that configuration. I'll look through the existing rules once. Thanks!

apb7 · 2018-03-19T20:40:28Z

core/templates/dev/head/components/CollectionCreationService.js

@@ -54,7 +54,7 @@ oppia.factory('CollectionCreationService', [
          }, function() {
            $rootScope.loadingMessage = '';
          }
-        );
+          );


Okay, got it!

apb7 · 2018-03-19T20:42:21Z

core/templates/dev/head/components/RatingDisplayDirective.js

-              ratingValue < $scope.stars[i].value - 0.75 ? 'fa-star-o' :
-              ratingValue < $scope.stars[i].value - 0.25 ? 'fa-star-half-o' :
-              'fa-star');
+                ratingValue < $scope.stars[i].value - 0.75 ? 'fa-star-o' :


Looking into this!

apb7 · 2018-03-19T20:47:41Z

core/templates/dev/head/components/StateGraphLayoutService.js

@@ -148,7 +148,7 @@ oppia.factory('StateGraphLayoutService', [
            if (numBacktrackingCalls <= MAX_BACKTRACKING_CALLS) {
              for (var i = 0; i < adjacencyLists[currentNodeId].length; i++) {
                if (currentPath.indexOf(
-                      adjacencyLists[currentNodeId][i]) === -1) {
+                  adjacencyLists[currentNodeId][i]) === -1) {


Sure, I have added it to my draft :)

apb7 · 2018-03-19T20:52:09Z

core/templates/dev/head/domain/exploration/OutcomeObjectFactorySpec.js

@@ -25,19 +25,19 @@ describe('Outcome object factory', function() {

  it('should correctly determine if an outcome is confusing given a ' +


We currently do not have any rule implemented for this. I am working on a custom rule to take care of this. I will make a separate PR for it. :)

apb7 · 2018-03-19T20:55:10Z

core/templates/dev/head/domain/exploration/OutcomeObjectFactorySpec.js

+    expect(testOutcome3.isConfusing(currentState)).toBe(false);
+    expect(testOutcome4.isConfusing(currentState)).toBe(true);
+    expect(testOutcome5.isConfusing(currentState)).toBe(true);
+  }


I am presently moving ) to the previous line. I'll look for a solution for it.

apb7 · 2018-03-19T21:05:11Z

core/templates/dev/head/pages/email_dashboard/EmailDashboardDataServiceSpec.js

+                           {
+                             id: 'q456',
+                             status: 'processing'
+                           }];


apb7 · 2018-03-19T21:05:22Z

core/templates/dev/head/pages/email_dashboard/EmailDashboardDataServiceSpec.js

+                             {
+                               id: 'q456',
+                               status: 'processing'
+                             }];


apb7 · 2018-03-19T21:06:20Z

core/templates/dev/head/pages/email_dashboard/EmailDashboardDataServiceSpec.js

+                           {
+                             id: 'q456',
+                             status: 'processing'
+                           }];


Same weird indent!

apb7 · 2018-03-19T21:09:51Z

extensions/interactions/GraphInput/directives/GraphInput.js

-            text: 'Weighted',
-            option: 'isWeighted'
-          }];
+                                 {


This is very weird! Looking into such indentations (I found a few above too). I am not too sure why this is occurring unless it is a false positive.

seanlip · 2018-03-20T07:10:43Z

Talk to the eslint folks perhaps? Not sure if this behaviour is intentional on their end. Worst case, we could write our own lint check.

apb7 · 2018-03-20T07:51:32Z

Hi @seanlip! I have filed Issue #10106 with eslint for this problem.
Thanks!

apb7 · 2018-03-20T19:05:04Z

Hi @seanlip! I have made the review changes. Can you please review this once again?
Thanks!

seanlip · 2018-03-20T23:21:18Z

core/templates/dev/head/components/forms/FormBuilder.js

-        $filter('convertUnicodeToHtml')(fragment.data) :
-        '<oppia-parameter>' + fragment.data +
+          $filter('convertUnicodeToHtml')(fragment.data) :
+          '<oppia-parameter>' + fragment.data +


Not sure I understand the indentation logic here.

Looking into the ternary operators. Thanks!

seanlip · 2018-03-20T23:22:38Z

core/templates/dev/head/domain/exploration/OutcomeObjectFactorySpec.js

@@ -25,19 +25,19 @@ describe('Outcome object factory', function() {

  it('should correctly determine if an outcome is confusing given a ' +


In the meantime shall we break it? Odd for the function() two lines below to have the same indentation level as it().

seanlip · 2018-03-20T23:22:54Z

core/templates/dev/head/domain/learner_dashboard/LearnerDashboardIdsBackendApiServiceSpec.js

+    expect(successHandler).not.toHaveBeenCalled();
+    expect(failHandler).toHaveBeenCalledWith(jasmine.objectContaining(
+      {data : 'Error loading dashboard IDs data.'}));
+  }


move ) below to end of this line?

Got it, thanks!

seanlip · 2018-03-20T23:23:41Z

core/templates/dev/head/filtersSpec.js

@@ -234,7 +234,7 @@ describe('Testing filters', function() {
      expect(filter(897978581123)).toEqual('898.0B');
      expect(filter(476678)).toEqual('476.7K');
    }
-  ));
+    ));


Ditto (and please check elsewhere too)

Sure, got it!

apb7

Hi @seanlip! I have noted down the review changes. I'm planning to close this PR and create a new one since I need to make changes to the existing rules. Does this sound good?
Thanks!

apb7 · 2018-03-21T22:01:40Z

extensions/interactions/GraphInput/directives/GraphInput.js

-            text: 'Weighted',
-            option: 'isWeighted'
-          }];
+                                 {


Yes, you're right. There must be some way to get around that configuration. I'll look through the existing rules once. Thanks!

seanlip · 2018-03-21T22:41:21Z

Hi @seanlip! I have noted down the review changes. I'm planning to close this PR and create a new one since I need to make changes to the existing rules. Does this sound good?

Sure, not a problem. Not sure I understand why you need to create a new PR, but I'm happy either way.

(Also, you don't really need to ask permission for things like this -- in general, feel free to handle PRs as you see fit. Thanks!)

apb7 · 2018-03-21T22:50:01Z

Most of the files have to be checked and fixed once again since the rules have to be modified though we can reset the changes but I think a PR on a fresh branch would be more easy.
Thanks :)

apb7 added 7 commits February 19, 2018 02:48

Merge pull request #13 from oppia/develop

28f20ed

Update Fork

Merge pull request #14 from oppia/develop

cdc2856

Update Fork

Merge pull request #15 from oppia/develop

af636ec

Update Fork

Merge pull request #16 from oppia/develop

860d44c

Update Fork

Merge pull request #19 from oppia/develop

0cdc197

Update Fork

Fix indent

90e6e0c

Change to alphabetical order

01911b5

apb7 requested a review from seanlip March 18, 2018 06:49

seanlip approved these changes Mar 18, 2018

View reviewed changes

Remove eslint-plugin-html

09c685c

Revert: Add eslint-plugin-html

89fde6c

Add ajv and ajv-keywords dependency

026e2eb

Remove ajv and ajv-keywords statements from install_third_party.sh

8f3c058

seanlip reviewed Mar 19, 2018

View reviewed changes

apb7 commented Mar 19, 2018

View reviewed changes

apb7 self-assigned this Mar 20, 2018

Review changes

e33f064

seanlip reviewed Mar 20, 2018

View reviewed changes

apb7 commented Mar 21, 2018

View reviewed changes

apb7 closed this Mar 21, 2018

		@@ -25,19 +25,19 @@ describe('Outcome object factory', function() {

		it('should correctly determine if an outcome is confusing given a ' +

Upgrade Eslint from 3.18.0 to 4.19.0 #4820

Upgrade Eslint from 3.18.0 to 4.19.0 #4820

Conversation

apb7 commented Mar 18, 2018

Explanation

Checklist

seanlip left a comment

Choose a reason for hiding this comment

apb7 commented Mar 18, 2018

apb7 commented Mar 18, 2018

apb7 commented Mar 18, 2018

apb7 commented Mar 18, 2018

codecov-io commented Mar 18, 2018 • edited

Codecov Report

kevinlee12 commented Mar 18, 2018

apb7 commented Mar 19, 2018 • edited

apb7 commented Mar 19, 2018

seanlip commented Mar 19, 2018 via email

apb7 commented Mar 19, 2018

apb7 commented Mar 19, 2018 • edited

apb7 commented Mar 19, 2018

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apb7 Mar 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Mar 20, 2018 via email

apb7 commented Mar 20, 2018

apb7 commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apb7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Mar 21, 2018

apb7 commented Mar 21, 2018

codecov-io commented Mar 18, 2018 •

edited

apb7 commented Mar 19, 2018 •

edited

apb7 commented Mar 19, 2018 •

edited

apb7 Mar 20, 2018 •

edited