Clean up work on constants. #6919

YashJipkate · 2019-06-13T19:49:14Z

Explanation

This PR is a clone of #6915.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
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 has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
The PR follows the style guide.
The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
- 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. Do not only request the review but also add the reviewer as an assignee.

vinitamurthi

Approving for question-player and practice-session files

YashJipkate · 2019-06-14T02:12:53Z

@seanlip @kevinlee12 The setup test shows pending here while it has passed as I saw in the dashboard:

Also, the frontend and backend tests are passing which implies that the setup test must have passed.

kevinlee12

lgtm!

I think we can probably override it since this is because Github isn't reporting the correct status.

core/templates/dev/head/pages/topic-editor-page/topic-editor-page.constants.ts

seanlip · 2019-06-14T05:18:20Z

No, we need to fix it so that GitHub reports the correct status. I am not overriding without all checks passing.

seanlip · 2019-06-14T05:35:06Z

Oh, that said, I am not too concerned about "setup" if the others do pass. (I made the original response via email.)

/cc @oppia/dev-workflow-team any idea why this is happening?

YashJipkate · 2019-06-14T05:42:48Z

@seanlip The other tests do pass! Could this be merged then?

seanlip

Hi @YashJipkate, took a pass, and I have a question. I think each constant should be declared exactly once in all of Oppia. I'm fine having a policy of "if a constant is used more than once, put it at app level", though there may be other better ways of handling this.

What are the policies that you would like to put in place, in order to achieve this? I would also like the presubmit checks that force this policy to be followed in the future to be part of this PR, so that devs don't (accidentally) undo the work you've done to separate them properly.

core/templates/dev/head/app.constants.ts

...mplates/dev/head/components/question-directives/question-editor/question-editor.constants.ts

core/templates/dev/head/pages/exploration-editor-page/exploration-editor-page.constants.ts

core/templates/dev/head/pages/exploration-player-page/exploration-player-page.constants.ts

core/templates/dev/head/pages/practice-session-page/practice-session-page.constants.ts

seanlip · 2019-06-14T08:33:38Z

@vojtechjelinek @Jamesjay4199 Flagging this comment, PTAL.

YashJipkate · 2019-06-16T07:57:13Z

@YashJipkate did you see this comment? #6919 (comment)

I have already addressed that comment, Sorry for not saying the same on the thread.

but I am not going to approve this until the INTERACTION_SPECS constant appears in only one place.

I thought that you meant this was something @Jamesjay4199 would have as a TODO. MIght be some misunderstanding of mine.
I have made a new file which has the constant and imported it wherever needed. PTAL!

YashJipkate · 2019-06-16T08:01:28Z

.github/CODEOWNERS

@@ -314,6 +314,8 @@
 /core/jobs*.py @seanlip
 /core/platform/ @seanlip
 /core/templates/dev/head/App*.ts @ankita240796
+/core/templates/dev/head/app.constants.ts @ankita240796
+/core/templates/dev/head/pages/interaction-specs.contants.ts @ankita240796


@seanlip I am not sure about this. Could you confirm?

I would say @vojtechjelinek (what do you think, @vojtechjelinek)?

scripts/pre_commit_linter.py

seanlip · 2019-06-16T08:04:31Z

I have already addressed that comment, Sorry for not saying the same on the thread.

But shouldn't that check have caught the case of INTERACTION_SPECS being repeated in the codebase, if it was working correctly?

YashJipkate · 2019-06-16T08:04:46Z

@seanlip The tests now pass (on my local). PTAL!

YashJipkate · 2019-06-16T08:05:27Z

But shouldn't that check have caught the case of interaction-specs being repeated in the codebase, if it was working correctly?

I had an exception for INTERACTION_SPECS in the linter so it is exempted.

scripts/pre_commit_linter.py

seanlip · 2019-06-16T09:10:23Z

.github/CODEOWNERS

@@ -314,6 +314,8 @@
 /core/jobs*.py @seanlip
 /core/platform/ @seanlip
 /core/templates/dev/head/App*.ts @ankita240796
+/core/templates/dev/head/app.constants.ts @ankita240796
+/core/templates/dev/head/pages/interaction-specs.contants.ts @ankita240796


I would say @vojtechjelinek (what do you think, @vojtechjelinek)?

seanlip · 2019-06-16T09:11:43Z

scripts/pre_commit_linter.py

@@ -347,6 +347,15 @@
    '/scripts/install_third_party.sh',
    '/.github/']

+OLD_CONVENTION_DIRECTORIES = [
+    'core/templates/dev/head/services',
+    'core/templates/dev/head/base_components',


Isn't this new? It wasn't around in the "old" codebase...

I don't think I understood you completely. These are the directories which follow the old convention and hence the name.

I guess I was thinking that base_components seems to have been recently added and is here to stay? I thought the idea was that all "new" dirs are cleaned up and we were going to remove all the dirs in this list completely over time... (which, btw, merits an issue or a TODO).

Are we planning to delete base_components too?

No, it doesn't need to be deleted. It just means that it is following the old convention. When we will empty this list out, it would mean that these directories now follow the new convention.

OK. I think you missed adding both an issue and a TODO for this (see previous comment). We need some way of keeping track of the task of "making this list empty".

Also add a comment above line 350 to explain to new devs what this constant is (in particular what is meant by "old convention").

I have added a comment as well a TODO. PTAL.

scripts/pre_commit_linter.py

oppiabot · 2019-06-16T16:11:20Z

Hi @YashJipkate. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

seanlip

Basically LGTM. Thanks @YashJipkate!

Is this PR blocking future work? If so, then if Travis tests pass you are welcome to incorporate the changes from here in the next PR instead, so that we can get this in.

.github/CODEOWNERS

seanlip · 2019-06-16T19:46:56Z

scripts/pre_commit_linter.py

@@ -347,6 +347,15 @@
    '/scripts/install_third_party.sh',
    '/.github/']

+OLD_CONVENTION_DIRECTORIES = [
+    'core/templates/dev/head/services',
+    'core/templates/dev/head/base_components',


OK. I think you missed adding both an issue and a TODO for this (see previous comment). We need some way of keeping track of the task of "making this list empty".

Also add a comment above line 350 to explain to new devs what this constant is (in particular what is meant by "old convention").

core/templates/dev/head/app.constants.ts

scripts/pre_commit_linter.py

YashJipkate · 2019-06-17T03:49:17Z

Hi @seanlip. No, this is not blocking any of my work currently (the one actually blocking is #6939) since work on Angular migration on constants can only begin after hybridization which is blocked by #6939. This PR is just a preparation for the next task after hybridization.

So I am fine addressing your review comments in this PR itself!
Thanks!

seanlip · 2019-06-17T06:01:06Z

Thanks @YashJipkate! Full LGTM from me. But please file an issue for #6919 (comment), assign it to yourself, and plan to take care of it as part of your project.

seanlip · 2019-06-17T06:01:36Z

Also since this change is mechanical I am happy to merge it once the Travis checks pass.

YashJipkate · 2019-06-17T06:17:53Z

@seanlip we already have an issue regarding separation of constants. See #6731. This PR fixes a part of that issue. So may be we can just update its description to apply this to the remaining folders?

seanlip · 2019-06-17T06:19:16Z

Sure, that's fine, as long as it's tracked. Are you planning to take that up?

(I don't want that list of folders to stay in the linter script as a long-term thing...)

YashJipkate · 2019-06-17T06:23:00Z

OK @seanlip I will try to do that right away in a follow up PR since that needs to be done sooner or later!

seanlip · 2019-06-17T06:23:31Z

Thanks, @YashJipkate! I will merge this one now.

YashJipkate · 2019-06-17T06:24:11Z

Also the tests pass. Could this be merged?

Edit: Thanks for merging!

Applied stash

9ca09e6

YashJipkate added the CHANGELOG: Infrastructural changes label Jun 13, 2019

YashJipkate requested review from aks681, ankita240796, DubeySandeep, kevinlee12, nithusha21, seanlip, vinitamurthi and vojtechjelinek as code owners June 13, 2019 19:49

YashJipkate assigned kevinlee12 and seanlip Jun 13, 2019

YashJipkate mentioned this pull request Jun 13, 2019

Clean up work on constants. #6915

Closed

8 tasks

vinitamurthi approved these changes Jun 14, 2019

View reviewed changes

kevinlee12 approved these changes Jun 14, 2019

View reviewed changes

core/templates/dev/head/pages/topic-editor-page/topic-editor-page.constants.ts Show resolved Hide resolved

seanlip requested changes Jun 14, 2019

View reviewed changes

seanlip assigned vojtechjelinek, jameesjohn and YashJipkate and unassigned kevinlee12 and seanlip Jun 14, 2019

YashJipkate added 3 commits June 14, 2019 15:30

Moved single use constants to page.constants

ecd35b1

Fix e2e by requiring the necessary constants file

8b153a8

Moved single use constants to services/ where they belong

d94f7fb

YashJipkate requested a review from brianrodri as a code owner June 14, 2019 12:08

Removed individual declaration of INTERACTION_SPECS

e16d48b

YashJipkate requested a review from seanlip June 16, 2019 07:57

YashJipkate commented Jun 16, 2019

View reviewed changes

scripts/pre_commit_linter.py Show resolved Hide resolved

YashJipkate commented Jun 16, 2019

View reviewed changes

scripts/pre_commit_linter.py Outdated Show resolved Hide resolved

seanlip requested changes Jun 16, 2019

View reviewed changes

YashJipkate added 2 commits June 16, 2019 15:15

Test

fa6b42b

Address Review

09f84a9

YashJipkate closed this Jun 16, 2019

YashJipkate reopened this Jun 16, 2019

Fix tests

26d1400

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Jun 16, 2019

Merge branch 'develop' into cl-sep-co

d313e05

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Jun 16, 2019

seanlip approved these changes Jun 16, 2019

View reviewed changes

Address review

20b7511

seanlip merged commit 4e665fa into oppia:develop Jun 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up work on constants. #6919

Clean up work on constants. #6919

YashJipkate commented Jun 13, 2019

vinitamurthi left a comment

YashJipkate commented Jun 14, 2019

kevinlee12 left a comment

seanlip commented Jun 14, 2019 via email

seanlip commented Jun 14, 2019

YashJipkate commented Jun 14, 2019

seanlip left a comment •

edited

Loading

seanlip commented Jun 14, 2019

YashJipkate commented Jun 16, 2019

YashJipkate Jun 16, 2019

seanlip Jun 16, 2019

seanlip commented Jun 16, 2019 •

edited

Loading

YashJipkate commented Jun 16, 2019

YashJipkate commented Jun 16, 2019

seanlip Jun 16, 2019

seanlip Jun 16, 2019

YashJipkate Jun 16, 2019

seanlip Jun 16, 2019

YashJipkate Jun 16, 2019 •

edited

Loading

seanlip Jun 16, 2019

YashJipkate Jun 17, 2019

oppiabot bot commented Jun 16, 2019

seanlip left a comment

seanlip Jun 16, 2019

YashJipkate commented Jun 17, 2019

seanlip commented Jun 17, 2019

seanlip commented Jun 17, 2019

YashJipkate commented Jun 17, 2019

seanlip commented Jun 17, 2019 •

edited

Loading

YashJipkate commented Jun 17, 2019

seanlip commented Jun 17, 2019

YashJipkate commented Jun 17, 2019 •

edited

Loading

Clean up work on constants. #6919

Clean up work on constants. #6919

Conversation

YashJipkate commented Jun 13, 2019

Explanation

Checklist

vinitamurthi left a comment

Choose a reason for hiding this comment

YashJipkate commented Jun 14, 2019

kevinlee12 left a comment

Choose a reason for hiding this comment

seanlip commented Jun 14, 2019 via email

seanlip commented Jun 14, 2019

YashJipkate commented Jun 14, 2019

seanlip left a comment • edited Loading

Choose a reason for hiding this comment

seanlip commented Jun 14, 2019

YashJipkate commented Jun 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jun 16, 2019 • edited Loading

YashJipkate commented Jun 16, 2019

YashJipkate commented Jun 16, 2019

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

YashJipkate Jun 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Jun 16, 2019

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YashJipkate commented Jun 17, 2019

seanlip commented Jun 17, 2019

seanlip commented Jun 17, 2019

YashJipkate commented Jun 17, 2019

seanlip commented Jun 17, 2019 • edited Loading

YashJipkate commented Jun 17, 2019

seanlip commented Jun 17, 2019

YashJipkate commented Jun 17, 2019 • edited Loading

seanlip left a comment •

edited

Loading

seanlip commented Jun 16, 2019 •

edited

Loading

YashJipkate Jun 16, 2019 •

edited

Loading

seanlip commented Jun 17, 2019 •

edited

Loading

YashJipkate commented Jun 17, 2019 •

edited

Loading