Added Mypy type annotations to the `core/controller` directory's files - 2. #16461

sahiljoster32 · 2022-11-01T09:18:18Z

Overview

This PR fixes or fixes part of N/A.
This PR does the following: This PR adds MyPy type annotations to the files of the core/controllers directory.

Essential 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 linter/Karma presubmit checks have passed locally on your machine.
"Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
- This lets reviewers restart your CircleCI tests for you.
The PR is made from a branch that's not called "develop".

Proof that changes are correct

Proof of MyPy checks that changes are correct.

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

Make sure to follow the instructions for making a code change.
If you need a review or an answer to a question, and don't have permissions to assign people, leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL". Oppiabot will help assign that person for you.
For what code owners will expect, see the Code Owner's wiki page.
Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
Never force push. If you do, your PR will be closed.
Some of the e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.

…controller-1

oppiabot · 2022-11-09T12:21:49Z

Unassigning @vojtechjelinek since they have already approved the PR.

oppiabot · 2022-11-10T02:31:39Z

Hi @sahiljoster32. 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!

sahiljoster32 · 2022-11-10T07:34:55Z

@seanlip @BenHenning PTAL!

seanlip

@sahiljoster32 The changes look great, just very minor nits left -- otherwise LGTM. Thanks!

seanlip · 2022-11-10T08:33:08Z

core/controllers/contributor_dashboard.py

+                for reference in topic.get_all_story_references()
+                if reference.story_is_published
+            ],
+                strict=True


Deindent this by 4, since it's in the (...) scope

seanlip · 2022-11-10T08:36:34Z

core/controllers/learner_goals.py

@@ -57,6 +57,12 @@ class LearnerGoalsHandler(
        'DELETE': {}
    }

+    # Note: Currently this handler only accepting one type of activity


accepting --> accepts

seanlip · 2022-11-10T08:36:45Z

core/controllers/learner_goals.py

@@ -57,6 +57,12 @@ class LearnerGoalsHandler(
        'DELETE': {}
    }

+    # Note: Currently this handler only accepting one type of activity
+    # which is 'ACTIVITY_TYPE_LEARN_TOPIC', so 'activity_type' argument
+    # is not used inside the function body but, in future if this handler


...inside the function body. But, in future, if this ...

seanlip · 2022-11-10T08:37:02Z

core/controllers/learner_goals.py

+    # Note: Currently this handler only accepting one type of activity
+    # which is 'ACTIVITY_TYPE_LEARN_TOPIC', so 'activity_type' argument
+    # is not used inside the function body but, in future if this handler
+    # accepts more than one activity of type then please remove pylint


...one activity type, then please remove the pylint exception below and use an 'if-else' ...

seanlip · 2022-11-10T08:37:09Z

core/controllers/learner_goals.py

@@ -74,6 +80,12 @@ def post(self, activity_type: str, topic_id: str) -> None:  # pylint: disable=un

        self.render_json(self.values)

+    # Note: Currently this handler only accepting one type of activity


Ditto as above.

sahiljoster32 · 2022-11-10T09:11:13Z

@seanlip PTAL!

seanlip

LGTM!

oppiabot · 2022-11-10T09:53:40Z

Unassigning @seanlip since they have already approved the PR.

oppiabot · 2022-11-10T09:53:44Z

Hi @sahiljoster32, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

BenHenning

Thanks @sahiljoster32. LGTM for Android compatibility codeowners.

oppiabot · 2022-11-10T11:44:00Z

Hi @sahiljoster32, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

sahiljoster32 · 2022-11-10T11:47:37Z

Since all the comments are addressed and every reviewer approved the PR, I'm going to merge this PR. Thanks!

sahiljoster32 added 30 commits October 15, 2022 23:48

added acl_decorator file

4b855a5

added some more files

3980a78

removed merge conflict

a0de77f

added backend fixes

c4a8e86

minor fixes

5dfa66a

added changes

500b5f9

Merge remote-tracking branch 'upstream/develop' into annotations-for-…

daa2a9e

…controller-1

self-review-1

16fb7b2

Merge remote-tracking branch 'upstream/develop' into annotations-for-…

1967830

…controller-1

added changes

1a0fb78

resolved merge conflict

1b42827

fixed backend coverage -1

7db9f39

added tests

030d539

fixes backend coverage -2

d8c7bad

added more coverage

1182b92

further increased coverage - 3

a174e9b

added changes

6e2f052

added changes

311fac1

added changes

5c8ad15

added changes and resolved merge conflict

2105f9f

added changes

97421f1

nits

1e03a25

nits

27d6640

added comments

48ab617

Merge remote-tracking branch 'upstream/develop' into annotations-for-…

61c76d9

…controller-1

added changes

fb80ccf

added changes

b2593b1

nits

6ada81e

added changes

b646014

added changes

a5a6c98

oppiabot bot unassigned vojtechjelinek Nov 9, 2022

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Nov 10, 2022

sahiljoster32 added 2 commits November 10, 2022 10:26

resolved merge conflict

acd4670

added changes

40c0ec7

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Nov 10, 2022

sahiljoster32 assigned seanlip and BenHenning and unassigned sahiljoster32 Nov 10, 2022

seanlip requested changes Nov 10, 2022

View reviewed changes

seanlip assigned sahiljoster32 and unassigned seanlip Nov 10, 2022

nits

dc82c26

sahiljoster32 assigned seanlip and unassigned sahiljoster32 Nov 10, 2022

seanlip approved these changes Nov 10, 2022

View reviewed changes

oppiabot bot unassigned seanlip Nov 10, 2022

oppiabot bot added the PR: LGTM label Nov 10, 2022

oppiabot bot assigned sahiljoster32 Nov 10, 2022

BenHenning approved these changes Nov 10, 2022

View reviewed changes

BenHenning removed their assignment Nov 10, 2022

sahiljoster32 merged commit ebe441e into oppia:develop Nov 10, 2022

AFZL210 mentioned this pull request Feb 14, 2024

Unable to visit the edit page for a public exploration when logged out #18370

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Mypy type annotations to the `core/controller` directory's files - 2. #16461

Added Mypy type annotations to the `core/controller` directory's files - 2. #16461

sahiljoster32 commented Nov 1, 2022

oppiabot bot commented Nov 9, 2022

oppiabot bot commented Nov 10, 2022

sahiljoster32 commented Nov 10, 2022

seanlip left a comment

seanlip Nov 10, 2022

sahiljoster32 Nov 10, 2022

seanlip Nov 10, 2022

sahiljoster32 Nov 10, 2022

seanlip Nov 10, 2022

sahiljoster32 Nov 10, 2022

seanlip Nov 10, 2022

sahiljoster32 Nov 10, 2022

seanlip Nov 10, 2022

sahiljoster32 Nov 10, 2022

sahiljoster32 commented Nov 10, 2022

seanlip left a comment

oppiabot bot commented Nov 10, 2022

oppiabot bot commented Nov 10, 2022

BenHenning left a comment

oppiabot bot commented Nov 10, 2022

sahiljoster32 commented Nov 10, 2022

		@@ -74,6 +80,12 @@ def post(self, activity_type: str, topic_id: str) -> None: # pylint: disable=un

		self.render_json(self.values)

		# Note: Currently this handler only accepting one type of activity

Added Mypy type annotations to the core/controller directory's files - 2. #16461

Added Mypy type annotations to the core/controller directory's files - 2. #16461

Conversation

sahiljoster32 commented Nov 1, 2022

Overview

Essential Checklist

Proof that changes are correct

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

oppiabot bot commented Nov 9, 2022

oppiabot bot commented Nov 10, 2022

sahiljoster32 commented Nov 10, 2022

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

sahiljoster32 commented Nov 10, 2022

seanlip left a comment

Choose a reason for hiding this comment

oppiabot bot commented Nov 10, 2022

oppiabot bot commented Nov 10, 2022

BenHenning left a comment

Choose a reason for hiding this comment

oppiabot bot commented Nov 10, 2022

sahiljoster32 commented Nov 10, 2022

Added Mypy type annotations to the `core/controller` directory's files - 2. #16461

Added Mypy type annotations to the `core/controller` directory's files - 2. #16461