Fix #6799: Add restricted globals rule to eslint #6923

ankita240796 · 2019-06-14T16:29:43Z

Explanation

Fixes #6799: Adds restricted globals rule to eslint and removes direct use of globals from codebase.

ankita240796 · 2019-06-14T16:31:45Z

extensions/interactions/MusicNotesInput/directives/MusicNotesInputRulesService.ts

@@ -43,7 +43,7 @@ oppia.factory('MusicNotesInputRulesService', [
      // TODO(wxy): validate that inputs.a <= inputs.b
      HasLengthInclusivelyBetween: function(answer, inputs) {
        var answerLength = _convertSequenceToMidi(answer).length;
-        return length >= inputs.a && length <= inputs.b;
+        return answerLength >= inputs.a && answerLength <= inputs.b;


Hi @seanlip, I am not sure if this change is right. There is no variable length declared in the function and we want to check if answerLength is inclusive between inputs.a and inputs.b (as from the function name and definition). So, was the variable length used here by mistake?

@DubeySandeep can you also please help me here, Thanks!

Goodness, yes, I think so. Really good catch. I wonder how it worked before ... :(

Just wondering, are there tests for this currently?

Yes, this PR adds test to avoid the use of global variables directly.

This was caught since length was flagged as a global variable used directly by eslint. Then, I realized that length here is used by mistake and is not a global variable actually.

Ah I meant tests for HasLengthInclusivelyBetween. Think it's worth adding them? They should be fairly trivial.

DubeySandeep

LGTM! (Left one simple comment!)

DubeySandeep · 2019-06-14T16:51:06Z

core/templates/dev/head/pages/exploration-editor-page/history-tab/history-tab.directive.ts

@@ -41,12 +41,12 @@ oppia.directive('historyTab', ['UrlInterpolationService', function(
      '/pages/exploration-editor-page/history-tab/history-tab.directive.html'),
    controllerAs: '$ctrl',
    controller: [
-      '$http', '$log', '$rootScope', '$scope',
+      '$http', '$log', '$rootScope', '$scope', '$window',


$window should go after $uibModal.

codecov · 2019-06-14T16:54:52Z

Codecov Report

Merging #6923 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #6923   +/-   ##
========================================
  Coverage    95.68%   95.68%           
========================================
  Files          371      371           
  Lines        51429    51429           
========================================
  Hits         49212    49212           
  Misses        2217     2217

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 3069fe6...0ee37cd. Read the comment docs.

codecov · 2019-06-14T16:54:53Z

Codecov Report

Merging #6923 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #6923   +/-   ##
========================================
  Coverage    95.68%   95.68%           
========================================
  Files          371      371           
  Lines        51429    51429           
========================================
  Hits         49212    49212           
  Misses        2217     2217

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 3069fe6...2933810. Read the comment docs.

seanlip · 2019-06-14T19:02:12Z

.eslintrc

@@ -73,6 +73,8 @@
      "error",
      "always"
    ],
+    "no-restricted-globals": [
+      "error", "addEventListener", "blur", "close", "closed", "confirm", "defaultStatus", "event", "external", "defaultstatus", "find", "focus", "frameElement", "frames", "history", "innerHeight", "innerWidth", "length", "location", "locationbar", "menubar", "moveBy", "moveTo", "name", "onblur", "onerror", "onfocus", "onload", "onresize", "onunload", "open", "opener", "opera", "outerHeight", "outerWidth", "pageXOffset", "pageYOffset", "parent", "print", "removeEventListener", "resizeBy", "resizeTo", "screen", "screenLeft", "screenTop", "screenX", "screenY", "scroll", "scrollbars", "scrollBy", "scrollTo", "scrollX", "scrollY", "self", "sessionStorage", "status", "statusbar", "stop", "toolbar", "top"],


Just to check, what is this list representing (and should it be so big? Wondering if we can remove anything.)

The list represents the common global variables which should not be used directly. I found the list from here.

I did not use the package directly since it is not maintained. Also, in the usage rules for this rule, the prescribed way was to add a list in eslintrc with the names of the globals we want to avoid. We can remove the globals which we don't think we need in the codebase. I am not sure how to find out such globals and remove from the list. Can you guide me here, Thanks!

Ah I see, so this is the list we want to avoid. In that case, that sounds good! I was worried that it was a list of what we were willing to accept, and was concerned that the list of exceptions seemed to be so big. No further change needed here, then.

Thanks @ankita240796 !

You're welcome!

Can we also add document, console etc. in this list as we expect to use $log, $document?

Hi folks -- a question for y'all. What's wrong with copying the list and maintaining our copy of it? It seems nice to not have to rely on any dependencies.

Yes, it seems fine to me. Just one concern -- we'll have to update our copy of the list after some interval probably, right?

Yes, if we plan to add some new globals. I am not sure if the eslint-restricted-globals package is being maintained a lot since the last commit was 7 months ago.

I think maintaining our own copy has its own pros and cons, but let's not dive into that. I'm okay with having our own copy, but maybe it would be nice to make an PR on the eslint-restricted-globals package (if the globals that we are adding are universal).

I agree with you on making a PR for the package. I will do that with the globals I have added in this PR as well.

seanlip

Couple small things, otherwise LGTM. I'll defer to you and @vojtechjelinek re the long comment thread on .eslintrc.

seanlip · 2019-06-14T22:23:39Z

.eslintrc

@@ -73,6 +73,8 @@
      "error",
      "always"
    ],
+    "no-restricted-globals": [
+      "error", "addEventListener", "blur", "close", "closed", "confirm", "defaultStatus", "event", "external", "defaultstatus", "find", "focus", "frameElement", "frames", "history", "innerHeight", "innerWidth", "length", "location", "locationbar", "menubar", "moveBy", "moveTo", "name", "onblur", "onerror", "onfocus", "onload", "onresize", "onunload", "open", "opener", "opera", "outerHeight", "outerWidth", "pageXOffset", "pageYOffset", "parent", "print", "removeEventListener", "resizeBy", "resizeTo", "screen", "screenLeft", "screenTop", "screenX", "screenY", "scroll", "scrollbars", "scrollBy", "scrollTo", "scrollX", "scrollY", "self", "sessionStorage", "status", "statusbar", "stop", "toolbar", "top"],


One separate suggestion -- please add line breaks to the list so that this file is cleanly viewable within an 80-char-width workspace.

seanlip · 2019-06-14T22:24:07Z

extensions/interactions/MusicNotesInput/directives/MusicNotesInputRulesService.ts

@@ -43,7 +43,7 @@ oppia.factory('MusicNotesInputRulesService', [
      // TODO(wxy): validate that inputs.a <= inputs.b
      HasLengthInclusivelyBetween: function(answer, inputs) {
        var answerLength = _convertSequenceToMidi(answer).length;
-        return length >= inputs.a && length <= inputs.b;
+        return answerLength >= inputs.a && answerLength <= inputs.b;


Ah I meant tests for HasLengthInclusivelyBetween. Think it's worth adding them? They should be fairly trivial.

kevinlee12

lgtm! (from code owner's)

ankita240796 · 2019-06-15T07:21:24Z

extensions/interactions/MusicNotesInput/directives/MusicNotesInputRulesService.ts

@@ -38,12 +38,12 @@ oppia.factory('MusicNotesInputRulesService', [
          _convertSequenceToMidi(inputs.x));
      },
      IsLongerThan: function(answer, inputs) {
-        return _convertSequenceToMidi(answer).length > inputs.x;
+        return _convertSequenceToMidi(answer).length > inputs.k;


Hi @seanlip, since I added test for the HasLengthInclusivelyBetween function, I also thought of adding tests of IsLongerThan since only these two functions did not have tests in the spec file. After looking into the function, I did not find this comparison correct as this was always returning false since inputs.x is not an integer field but a list instead.

The format for inputs.x is:

x: [{ readableNoteName: 'A4', noteDuration: { num: 1, den: 1 } }]

I tried to see the exploration yaml by creating and downloading an exploration but it did not work due to #6931.

So, I checked the file pitch_perfect.yaml where the rule isLongerThan requires inputs.k. You can check here.

I am not a lot familiar with this interaction. So, can you please confirm if this change is correct. Thanks!

Yes, you are correct. Thank you very much for checking and for doing this!

vojtechjelinek

LGTM from code owner's perspective. Thanks!

seanlip

LGTM, thanks @ankita240796! All changes look great.

@apb7 this PR needs your review as codeowner. PTAL!

seanlip · 2019-06-16T03:37:03Z

extensions/interactions/MusicNotesInput/directives/MusicNotesInputRulesService.ts

@@ -38,12 +38,12 @@ oppia.factory('MusicNotesInputRulesService', [
          _convertSequenceToMidi(inputs.x));
      },
      IsLongerThan: function(answer, inputs) {
-        return _convertSequenceToMidi(answer).length > inputs.x;
+        return _convertSequenceToMidi(answer).length > inputs.k;


Yes, you are correct. Thank you very much for checking and for doing this!

apb7

LGTM, thanks @ankita240796!

ankita240796 added 2 commits June 14, 2019 20:28

Add no-restricted-globals

89bcb07

Remove globals

07197e1

ankita240796 requested review from apb7, DubeySandeep, kevinlee12, seanlip and vojtechjelinek as code owners June 14, 2019 16:29

ankita240796 commented Jun 14, 2019

View reviewed changes

DubeySandeep approved these changes Jun 14, 2019

View reviewed changes

Fix order of dependencies

2933810

Fix test

0ee37cd

seanlip reviewed Jun 14, 2019

View reviewed changes

seanlip approved these changes Jun 14, 2019

View reviewed changes

seanlip assigned kevinlee12, vojtechjelinek, ankita240796 and apb7 Jun 14, 2019

kevinlee12 approved these changes Jun 15, 2019

View reviewed changes

kevinlee12 removed their assignment Jun 15, 2019

ankita240796 added 3 commits June 15, 2019 12:36

Add linebreaks

c00bda0

Add test

ea0a8b9

Merge remote-tracking branch 'upstream/develop' into restrict-globals

e8989f5

ankita240796 commented Jun 15, 2019

View reviewed changes

ankita240796 assigned seanlip and unassigned ankita240796 Jun 15, 2019

vojtechjelinek approved these changes Jun 15, 2019

View reviewed changes

vojtechjelinek removed their assignment Jun 15, 2019

seanlip approved these changes Jun 16, 2019

View reviewed changes

seanlip removed their assignment Jun 16, 2019

apb7 approved these changes Jun 16, 2019

View reviewed changes

seanlip merged commit 4802241 into oppia:develop Jun 16, 2019

ankita240796 deleted the restrict-globals branch June 16, 2019 04:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #6799: Add restricted globals rule to eslint #6923

Fix #6799: Add restricted globals rule to eslint #6923

ankita240796 commented Jun 14, 2019

ankita240796 Jun 14, 2019 •

edited

Loading

ankita240796 Jun 14, 2019

seanlip Jun 14, 2019

ankita240796 Jun 14, 2019

ankita240796 Jun 14, 2019

seanlip Jun 14, 2019

ankita240796 Jun 15, 2019

DubeySandeep left a comment

DubeySandeep Jun 14, 2019

ankita240796 Jun 14, 2019

codecov bot commented Jun 14, 2019 •

edited

Loading

codecov bot commented Jun 14, 2019

seanlip Jun 14, 2019

ankita240796 Jun 14, 2019

seanlip Jun 14, 2019

ankita240796 Jun 14, 2019

DubeySandeep Jun 14, 2019

seanlip Jun 15, 2019

apb7 Jun 15, 2019

ankita240796 Jun 15, 2019

vojtechjelinek Jun 15, 2019

ankita240796 Jun 15, 2019

seanlip left a comment

seanlip Jun 14, 2019

ankita240796 Jun 15, 2019

seanlip Jun 14, 2019

kevinlee12 left a comment

ankita240796 Jun 15, 2019 •

edited

Loading

seanlip Jun 16, 2019

vojtechjelinek left a comment

seanlip left a comment

seanlip Jun 16, 2019

apb7 left a comment

Fix #6799: Add restricted globals rule to eslint #6923

Fix #6799: Add restricted globals rule to eslint #6923

Conversation

ankita240796 commented Jun 14, 2019

Explanation

ankita240796 Jun 14, 2019 • edited Loading

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

DubeySandeep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 14, 2019 • edited Loading

Codecov Report

codecov bot commented Jun 14, 2019

Codecov Report

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 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

kevinlee12 left a comment

Choose a reason for hiding this comment

ankita240796 Jun 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechjelinek left a comment

Choose a reason for hiding this comment

seanlip left a 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

ankita240796 Jun 14, 2019 •

edited

Loading

codecov bot commented Jun 14, 2019 •

edited

Loading

ankita240796 Jun 15, 2019 •

edited

Loading