Improving the dev workflow: Milestone 1.2.3 Enable checks for directive scope #4980

apb7 · 2018-05-25T11:44:14Z

Explanation

Fixes #4955

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

Update fork

Update Fork

Update fork

codecov-io · 2018-05-25T16:12:59Z

Codecov Report

Merging #4980 into develop will increase coverage by 0.12%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #4980      +/-   ##
===========================================
+ Coverage    45.04%   45.16%   +0.12%     
===========================================
  Files          392      397       +5     
  Lines        23705    23789      +84     
  Branches      3848     3849       +1     
===========================================
+ Hits         10678    10745      +67     
- Misses       13027    13044      +17

Impacted Files	Coverage Δ
extensions/objects/templates/GraphEditor.js	`11.11% <ø> (ø)`	⬆️
extensions/objects/templates/HtmlEditor.js	`11.11% <ø> (ø)`	⬆️
core/templates/dev/head/directives.js	`22.05% <ø> (ø)`	⬆️
extensions/value_generators/templates/Copier.js	`7.14% <0%> (+0.47%)`	⬆️
...ns/PencilCodeEditor/directives/PencilCodeEditor.js	`32.32% <0%> (ø)`	⬆️
...ions/objects/templates/LogicErrorCategoryEditor.js	`6.25% <0%> (ø)`	⬆️
...ons/objects/templates/ListOfUnicodeStringEditor.js	`9.09% <0%> (ø)`	⬆️
...dev/head/components/forms/ObjectEditorDirective.js	`7.14% <0%> (-5.36%)`	⬇️
extensions/objects/templates/CodeStringEditor.js	`5.55% <0%> (ø)`	⬆️
...xploration_editor/ValueGeneratorEditorDirective.js	`7.69% <0%> (-6.6%)`	⬇️
... and 27 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 9f27d9b...baf53e4. Read the comment docs.

apb7 · 2018-06-04T17:34:34Z

@seanlip and @kevinlee12: I have added a regex check for $scope.$parent. The only problem I am facing at the moment is the removal of $scope.$parent statements from RandomSelector and Copier. The pattern checks, therefore, will fail at the moment. I have tested the object templates locally.
Thanks!

seanlip · 2018-06-04T19:06:33Z

Can you explain your debugging process for RandomSelector and Copier, similar to what I showed you for how to debug the issue with the object templates? It would help to understand how far you've gotten.

kevinlee12 · 2018-06-05T04:53:05Z

core/templates/dev/head/pages/exploration_editor/ValueGeneratorEditorDirective.js

+          return scope.objType;
+        };
+        element.html('<' + directiveName +
+        ' customization-args="customizationArgs"' +


Don't forget the spacing :)

Done, thanks!

I meant the indentation :)

Whoops! Fixed.

apb7

@kevinlee12 and @seanlip: I have fixed all directive scopes now. Just a single doubt. Thanks!

apb7 · 2018-06-05T05:01:36Z

core/templates/dev/head/pages/exploration_editor/ValueGeneratorEditorDirective.js

+          return scope.objType;
+        };
+        element.html('<' + directiveName +
+        ' customization-args="customizationArgs"' +


Done, thanks!

apb7 · 2018-06-05T05:03:42Z

extensions/value_generators/templates/Copier.js

      }, true);

      $scope.$watch('$parent.objType', function() {
-        $scope.objType = $scope.$parent.objType;
+        $scope.objType = $scope.getObjType();


I had a doubt here. How do we modify lines 47 and 51 to remove $scope.$parent?

I don't know, but it looks like you might have addressed this :) (Though this needs careful and thorough manual testing of get/update/etc. operations.)

In general try to get rid of the $parent in the line above too, and have a lint check to forbid use of $parent anywhere (not just scope.$parent).

Thanks!

…irective-scope-check

kevinlee12 · 2018-06-06T04:05:39Z

Functionality and code, it looks good up to this point, though please check if you can remove $parent from app.js.

kevinlee12

lgtm, thanks for tackling this one!

apb7 · 2018-06-06T04:48:56Z

Discussed with @seanlip. I have filed Issue #5040 for this. Thanks!

kevinlee12 · 2018-06-06T04:49:46Z

@apb7, feel free to merge once tests pass.

apb7 · 2018-06-06T15:43:08Z

Merging, thanks @kevinlee12!

…ve scope (oppia#4980) * isolate RandomSelector scope * change RandomSelector & Copier scope * Fix directive scope * modify scopes * isolate scopes * isolate scopes * isolate scopes(3) * Revert and fix * Run all e2e * isolate UnicodeStringEditor scope * Run all tests * isolate NormalizedStringEditor * isolate SanitizedUrlEditor * isolate BooleanEditor * isolate CodeStringEditor * isolate FractionEditor * isolate GraphEditor and GraphPropertyEditor * isolate HtmlEditor * Modify previous scopes, isolate IntEditor, ListOfUnicodeStringEditor * isolate LogicQuestionEditor * isolate LogicErrorCategoryEditor * isolate ImageWithRegionsEditor * isolate MathLatexStringEditor * isolate MusicPhraseEditor * isolate NonnegativeIntEditor * isolate ParameterNameEditor * isolate RealEditor * isolate SetOfHtmlStringEditor * isolate SetOfUnicodeStringEditor * isolate CoordTwoDimEditor * isolate FilepathEditor * Remove optional binding * isolate mobileFriendlyTooltip * isolate Copier * Revert: isolate Copier * isolate RandomSelector * isolate Copier * Fix SetOfHtmlStringEditor * Alphabetize scopes * Fix SetOfHtmlStringEditor * Fix ImageWithRegionsEditor * Fix GraphEditor * Fix SetOfHtmlStringEditor * Add regex check for . * Passed values via ObjectEditorDirective and made corresponding fixes * Remove parent from FractionEditor * Add functions to pass other values * Remove . from editor objects and fix scopes * Fix hanging indentation * Remove scope.parent statement from PencilCodeEditor * Fix Copier and RandomSelector directives * Remove scope.parent from RandomSelector * Add spaces * Remove watch blocks from Copier * Modify NumberWithUnitsEditor * Enable directive scope check * Remove parent from Copier * Add parent regexp in pre-commit script * Remove parent from directives * Fix indentation

apb7 added 19 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

Merge pull request #20 from oppia/develop

bf888ba

Update Fork

Merge pull request #21 from oppia/develop

5e546fd

Update fork

Merge pull request #22 from oppia/develop

9859cd2

Update fork

Merge pull request #23 from oppia/develop

e90fcde

Update fork

Merge pull request #24 from oppia/develop

b7b69fb

Update fork

Merge pull request #25 from oppia/develop

108bd8d

Update fork

Merge pull request #26 from oppia/develop

f165670

Update Fork

Merge pull request #27 from oppia/develop

759e4da

Update fork

Merge pull request #28 from oppia/develop

420df20

Update fork

Merge pull request #29 from oppia/develop

0fb552c

Update fork

Merge pull request #30 from oppia/develop

91dfed9

Update fork

Merge pull request #31 from oppia/develop

c1f8fea

Update fork

Merge pull request #32 from oppia/develop

16af003

Update fork

isolate RandomSelector scope

ac67fa8

apb7 self-assigned this May 25, 2018

apb7 added 2 commits May 25, 2018 19:06

change RandomSelector & Copier scope

9c8d407

Fix directive scope

835e6ff

apb7 added 7 commits May 25, 2018 22:36

modify scopes

2f350a9

isolate scopes

7490b24

isolate scopes

3b7e0e7

isolate scopes(3)

abacc7b

Revert and fix

6d53711

Run all e2e

593415c

isolate UnicodeStringEditor scope

b4e42c4

apb7 added 6 commits June 4, 2018 19:38

Passed values via ObjectEditorDirective and made corresponding fixes

7b70930

Remove parent from FractionEditor

058b0ef

Add functions to pass other values

6624efd

Remove . from editor objects and fix scopes

0f97b15

Fix hanging indentation

9f9ef1d

Remove scope.parent statement from PencilCodeEditor

b28c3c7

apb7 added 2 commits June 5, 2018 10:04

Fix Copier and RandomSelector directives

7f91847

Remove scope.parent from RandomSelector

d7d3eb4

kevinlee12 reviewed Jun 5, 2018

View reviewed changes

Add spaces

a4bc98f

apb7 commented Jun 5, 2018

View reviewed changes

apb7 added 7 commits June 5, 2018 10:48

Remove watch blocks from Copier

1054ec9

Merge branch 'develop' of https://github.com/apb7/oppia into enable-d…

3474741

…irective-scope-check

Modify NumberWithUnitsEditor

895b64f

Enable directive scope check

ed52fd2

Remove parent from Copier

2715926

Add parent regexp in pre-commit script

10f460d

Remove parent from directives

e7886fb

Fix indentation

baf53e4

kevinlee12 approved these changes Jun 6, 2018

View reviewed changes

apb7 merged commit a06a0bd into oppia:develop Jun 6, 2018

apb7 deleted the enable-directive-scope-check branch June 6, 2018 15:43

apb7 mentioned this pull request Jun 9, 2018

Remove $parent from HTML files #5069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving the dev workflow: Milestone 1.2.3 Enable checks for directive scope #4980

Improving the dev workflow: Milestone 1.2.3 Enable checks for directive scope #4980

apb7 commented May 25, 2018 •

edited

Loading

codecov-io commented May 25, 2018 •

edited

Loading

apb7 commented Jun 4, 2018

seanlip commented Jun 4, 2018

kevinlee12 Jun 5, 2018

apb7 Jun 5, 2018

kevinlee12 Jun 6, 2018

apb7 Jun 6, 2018

apb7 left a comment

apb7 Jun 5, 2018

apb7 Jun 5, 2018

seanlip Jun 5, 2018

kevinlee12 commented Jun 6, 2018

kevinlee12 left a comment

apb7 commented Jun 6, 2018

kevinlee12 commented Jun 6, 2018

apb7 commented Jun 6, 2018

Improving the dev workflow: Milestone 1.2.3 Enable checks for directive scope #4980

Improving the dev workflow: Milestone 1.2.3 Enable checks for directive scope #4980

Conversation

apb7 commented May 25, 2018 • edited Loading

Explanation

Checklist

codecov-io commented May 25, 2018 • edited Loading

Codecov Report

apb7 commented Jun 4, 2018

seanlip commented Jun 4, 2018

kevinlee12 Jun 5, 2018

Choose a reason for hiding this comment

apb7 Jun 5, 2018

Choose a reason for hiding this comment

kevinlee12 Jun 6, 2018

Choose a reason for hiding this comment

apb7 Jun 6, 2018

Choose a reason for hiding this comment

apb7 left a comment

Choose a reason for hiding this comment

apb7 Jun 5, 2018

Choose a reason for hiding this comment

apb7 Jun 5, 2018

Choose a reason for hiding this comment

seanlip Jun 5, 2018

Choose a reason for hiding this comment

kevinlee12 commented Jun 6, 2018

kevinlee12 left a comment

Choose a reason for hiding this comment

apb7 commented Jun 6, 2018

kevinlee12 commented Jun 6, 2018

apb7 commented Jun 6, 2018

apb7 commented May 25, 2018 •

edited

Loading

codecov-io commented May 25, 2018 •

edited

Loading