Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix part of #14033: Added Mypy type annotations to some files. #14469

Merged
merged 53 commits into from
Jan 19, 2022
Merged

Fix part of #14033: Added Mypy type annotations to some files. #14469

merged 53 commits into from
Jan 19, 2022

Conversation

sahiljoster32
Copy link
Contributor

Overview

  1. This PR fixes or fixes part of Adding Mypy type annotations to domain folder files #14033 .
  2. This PR does the following: Added annotations to config_domian.py and caching_services.py

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

No proof of changes needed because following tests passed on local machine.

  • lint checks.
  • mypy type checks

PR Pointers

  • Make sure to follow the instructions for making a code change.
  • Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
  • 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.
  • Oppiabot can assign anyone for review/help if you leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL"
  • 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.

srijanreddy98 and others added 17 commits November 26, 2021 23:19
@srijanreddy98
Fixes part of #9749: Migrate few instances of angular-html-bind (#14263)
d74038b 
* Revert "Revert "Fix part of #9749: Create a component that shows rte-output-display and use it instead of angular-html-bind. (#13229)" (#13361)"

This reverts commit 792de45.

* Fix issues

* Fix spelling mistakes in comment

* Merge with upstream/develop

* Address review comments:
  1. Move oppia-rte-output.spec from the list of ignored files for innerHTML check in eslintrc and move to inline disables.
  2. Change the CODEOWNER listing from oppia-rte-parser.service.ts to oppia-rte-parser.service*.ts to include the spec file.
  3. Add refernce to the note at the top of file for all value.replace ops.
  4. Miscellaneous changes.

Co-authored-by: Vasa Srijan Reddy <redvasa@amazon.com>
@sahiljoster32
Merge remote-tracking branch 'upstream/develop' into develop
354d901
@sahiljoster32
Merge remote-tracking branch 'upstream/develop' into develop
7930e4f
@sahiljoster32
Merge remote-tracking branch 'upstream/develop' into develop
2ff1fcb
@sahiljoster32
Merge remote-tracking branch 'upstream/develop' into develop
7da71af
@sahiljoster32
Merge remote-tracking branch 'upstream/develop' into develop
6afee7e
@sahiljoster32
Merge remote-tracking branch 'upstream/develop' into develop
f6a3c12
@sahiljoster32
pending with tests
5dc96d8
@sahiljoster32
pending with caching_services
bffe408
@sahiljoster32
lint 1
f65c8ba
@sahiljoster32
lint 2
39fbb95
@sahiljoster32
lint 3
c34365a
@sahiljoster32
pending with test
7fec54c
@sahiljoster32
lint
1885375
@sahiljoster32
done
9667b75
@sahiljoster32
fixed models type
13f1b09
@sahiljoster32
nits
8a67ca0
@sahiljoster32 sahiljoster32 requested a review from a team December 21, 2021 13:23
@oppiabot
Copy link

oppiabot bot commented Dec 21, 2021

Hi, @sahiljoster32, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR pointers. Assigning @sahiljoster32 to add the required label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

@sahiljoster32
Copy link
Contributor Author

@vojtechjelinek PTAL!

seanlip
seanlip approved these changes Jan 17, 2022
View reviewed changes
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for 1 codeowner file

@seanlip seanlip removed their assignment Jan 17, 2022
@Hudda Hudda removed their assignment Jan 17, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 17, 2022

Unassigning @DubeySandeep since they have already approved the PR.

@sahiljoster32
Copy link
Contributor Author

@vojtechjelinek PTAL!

@oppiabot oppiabot bot assigned vojtechjelinek and unassigned sahiljoster32 Jan 18, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 18, 2022

Unassigning @sahiljoster32 since a re-review was requested. @sahiljoster32, please make sure you have addressed all review comments. Thanks!

vojtechjelinek
vojtechjelinek approved these changes Jan 18, 2022
View reviewed changes
Copy link
Contributor

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I just rephrased the comment a bit.

.coveragerc Outdated Show resolved Hide resolved
@vojtechjelinek vojtechjelinek removed their assignment Jan 18, 2022
aks681
aks681 approved these changes Jan 18, 2022
View reviewed changes
Copy link
Contributor

@aks681 aks681 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm as codeowner

@sahiljoster32 @vojtechjelinek
corrected comment
085e97e 
Co-authored-by: Vojtěch Jelínek <vojtech.jelinek@hey.com>
@aks681 aks681 removed their assignment Jan 18, 2022
@sahiljoster32
Copy link
Contributor Author

@hardikkat24 PTAL!

hardikkat24
hardikkat24 approved these changes Jan 19, 2022
View reviewed changes
Copy link
Contributor

@hardikkat24 hardikkat24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. LGTM for codeowner files.

@oppiabot
Copy link

oppiabot bot commented Jan 19, 2022

Unassigning @hardikkat24 since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Jan 19, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 19, 2022

Hi @vojtechjelinek, this PR is ready to be merged. Author of this PR does not have permissions to merge this PR. Before you merge it, please make sure that there are no pending comments that require action from the author's end. Thanks!

@vojtechjelinek vojtechjelinek merged commit 8e54835 into oppia:develop Jan 19, 2022
@sahiljoster32 sahiljoster32 deleted the mytypes branch January 19, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hardikkat24 hardikkat24 hardikkat24 approved these changes

@Hudda Hudda Hudda approved these changes

@aks681 aks681 aks681 approved these changes

@DubeySandeep DubeySandeep DubeySandeep approved these changes

@seanlip seanlip seanlip approved these changes

@vojtechjelinek vojtechjelinek vojtechjelinek approved these changes

Assignees

@vojtechjelinek vojtechjelinek

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sahiljoster32 @vojtechjelinek @seanlip @DubeySandeep @aks681 @Hudda @hardikkat24 @srijanreddy98