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.

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

Fix 9996 and Fix part of #9982: Refactor backend api services and remove remaining any types #10054

Merged
merged 8 commits into from Jul 27, 2020
Merged

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented Jul 26, 2020

Overview

  1. This PR fixes Remove the AngularJs native variables which are typed as any #9996, part of Irregularity in errorCallback in backend-api services #9982
  2. This PR does the following: Refactor backend api services and remove remaining any types

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

PR Pointers

  • 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
Copy link

oppiabot bot commented Jul 26, 2020

Assigning @ankita240796 for the first-pass review of this pull request. Thanks!

return new Promise((resolve, reject) => {
this.http.get<AdminPageDataBackendDict>(
AdminPageConstants.ADMIN_HANDLER_URL).toPromise().then(response => {
resolve({
demoExplorations: response.demo_explorations,
demoCollections: response.demo_collections,
demoExplorationIds: response.demo_exploration_ids,
oneOffJobStatusSummaries: response.one_off_job_status_summaries.map(
this.jobStatusSummaryObjectFactory.createFromBackendDict),
humanReadableCurrentTime: response.human_readable_current_time,
auditJobStatusSummaries: response.audit_job_status_summaries.map(
this.jobStatusSummaryObjectFactory.createFromBackendDict),
updatableRoles: response.updatable_roles,
roleGraphData: response.role_graph_data,
configProperties: response.config_properties,
viewableRoles: response.viewable_roles,
unfinishedJobData: response.unfinished_job_data.map(
this.jobDataObjectFactory.createFromBackendDict),
recentJobData: response.recent_job_data.map(
this.jobDataObjectFactory.createFromBackendDict),
continuousComputationsData: response.continuous_computations_data.map(
this.computationDataObjectFactory.createFromBackendDict),
topicSummaries: response.topic_summaries.map(
this.topicSummaryObjectFactory.createFromBackendDict)
});
}, errorResponse => {
reject(errorResponse.error.error);
});
Copy link
Contributor Author

@nishantwrp nishantwrp Jul 26, 2020

Choose a reason for hiding this comment

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

I've refactored

return this.http.get(...).then(response => {
  ...
  return something;
}, error {
  throw new Error(error.error.error);
});

to

return new Promise((resolve, reject) => {
  this.http.get(...).then(response => {
    ...
    resolve(something);
  }, error {
    reject(error.error.error);
  });
});

Because the first one when rejecting the promise gives an instance of Error object. but the second one gives a string. In almost every other places in the codebase we return a string in case of a rejection. So, I did this to bring consistency. Also in many places we use AlertsService to throw errors from backend. This makes it easy to do that.

Copy link
Member

@seanlip seanlip left a comment

Thanks @nishantwrp! Couple of comments (took a look only at the codeowner files).

One thought -- think about how to prevent these issues in the future, and put steps in place to do that. E.g. data.error, error.error -- is this standardized throughout the entire codebase now? Do we have documentation anywhere? How do we ensure this still holds going forward?

Also one other note -- can you get rid of the "?"s generally? They should only be used when null is a possible value, right? I remember in #9919 there was some issue with this.

topicSummaries: response.topic_summaries.map(
this.topicSummaryObjectFactory.createFromBackendDict)
};
return new Promise((resolve, reject) => {
Copy link
Member

@seanlip seanlip Jul 26, 2020

Choose a reason for hiding this comment

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

Should resolve, reject have types?

Also, what is the general plan for ensuring that everything is fully typed?

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 26, 2020

Choose a reason for hiding this comment

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

They have types already. (They are inferred automatically by the ts compiler)

Copy link
Member

@seanlip seanlip Jul 26, 2020

Choose a reason for hiding this comment

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

Ah ok, thanks. One question to clarify then: does the TS compiler complain if anything isn't fully typed?

Also can you add some info about this "inferring" stuff to the wiki page? I think contributors need to understand the cases in which we need to add types and those in which it's not necessary to do so.

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 26, 2020

Choose a reason for hiding this comment

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

Ah ok, thanks. One question to clarify then: does the TS compiler complain if anything isn't fully typed?

Not yet, but we can enable a check after the angular migration. (Because then everything will have types.)

Also can you add some info about this "inferring" stuff to the wiki page? I think contributors need to understand the cases in which we need to add types and those in which it's not necessary to do so.

Okay.

Copy link
Member

@seanlip seanlip Jul 26, 2020

Choose a reason for hiding this comment

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

Why do you need to wait for the Angular migration for everything to have types?

Can't things have types before the Angular migration? We can't really consider types to be fully in place until types are enforced universally, and the Angular migration is going to take a while.

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 26, 2020

Choose a reason for hiding this comment

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

Because the structure of the angular js files does not favor typing. There are no classes. But a single

angular.module('oppia', ..., function(...) {  })

that contains all the stuff.

So, defining types for angular js files is not possible. I even discussed this thing in my GSoC Proposal(https://docs.google.com/document/d/1fIoXVtO3Br2BtuR2B8XnsmJsHTAd3SjsGhUbjENJuPc/edit#bookmark=id.qceznp891cbq).

Copy link
Member

@seanlip seanlip Jul 26, 2020

Choose a reason for hiding this comment

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

OK, noted. Thanks (and sorry for forgetting that!)

That said -- a different question. Can the TS compiler complain if anything isn't fully typed, for migrated files only?

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 26, 2020

Choose a reason for hiding this comment

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

Nope because it there is no check for that because there is no way it can know if the file is in angular or angular js.

Copy link
Member

@seanlip seanlip Jul 26, 2020

Choose a reason for hiding this comment

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

OK, thanks. Let's move this to an email thread to see what's possible (it won't block the merge of this PR).

// TODO(#9996): Replace 'any' with the exact type. This has kept as
// 'any' since canceler is a 'Deferred' type object which is native to
// AngularJS and does not have a type in native typescript.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Member

@seanlip seanlip Jul 26, 2020

Choose a reason for hiding this comment

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

Can you add a lint check that disallows "no-explicit-any" anywhere in the codebase? Probably just adding it to the list of forbidden strings suffices (but make sure it applies to the right files).

In general, think about how to ensure that the measures you are putting in place now will hold forever.

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 26, 2020

Choose a reason for hiding this comment

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

done

@seanlip seanlip assigned nishantwrp and unassigned seanlip Jul 26, 2020
@nishantwrp nishantwrp requested a review from Hudda as a code owner Jul 26, 2020
aks681
aks681 approved these changes Jul 26, 2020
Copy link
Member

@aks681 aks681 left a comment

Lgtm for 4 codeowner files

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

LGTM!

Copy link
Member

@seanlip seanlip left a comment

Thanks @nishantwrp -- just the inline comments, and the general top-level comment that I left in the previous review are left, I think.

r'eslint-(disable|enable)(-next-line)?'
' @typescript-eslint/no-explicit-any'),
'message': (
'Please do not define any types. You can refer '
Copy link
Member

@seanlip seanlip Jul 26, 2020

Choose a reason for hiding this comment

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

any --> "any"

Great idea to refer to the wiki page -- thanks!

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 26, 2020

Choose a reason for hiding this comment

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

done

},
{
'regexp': re.compile(
r'eslint-(disable|enable)(-next-line)?'
Copy link
Member

@seanlip seanlip Jul 26, 2020

Choose a reason for hiding this comment

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

Don't think you need all the surrounding stuff -- just have a check for 'no-explicit-any'? That will catch more stuff.

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 26, 2020

Choose a reason for hiding this comment

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

done

@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jul 26, 2020

@seanlip

One thought -- think about how to prevent these issues in the future, and put steps in place to do that. E.g. data.error, error.error -- is this standardized throughout the entire codebase now? Do we have documentation anywhere? How do we ensure this still holds going forward?

Maybe we can have a wiki page for style guide for the backend API services. WDYT?

Also one other note -- can you get rid of the "?"s generally? They should only be used when null is a possible value, right? I remember in #9919 there was some issue with this.

Oh, okay. Yup, I'll do that but a bit later because I will have to check which ? are really required what which can be removed.

@nishantwrp nishantwrp requested a review from seanlip Jul 26, 2020
@nishantwrp nishantwrp assigned seanlip and unassigned nishantwrp Jul 26, 2020
@seanlip
Copy link
Member

seanlip commented Jul 26, 2020

Maybe we can have a wiki page for style guide for the backend API services. WDYT?

As usual -- anything works as long as it achieves the desired outcome. This seems like a reasonable start but more may be needed.

Oh, okay. Yup, I'll do that but a bit later because I will have to check which ? are really required what which can be removed.

Yup, agree this should be in a separate PR. Could you please file an issue for it and self-assign it?

Thanks!

@seanlip
Copy link
Member

seanlip commented Jul 26, 2020

(Link to that issue here.)

@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jul 26, 2020

Filed #10055

Copy link
Member

@seanlip seanlip left a comment

Thanks @nishantwrp. LGTM for codeowner files!

Copy link
Member

@nithusha21 nithusha21 left a comment

LGTM for codeowner files

@nithusha21 nithusha21 removed their assignment Jul 27, 2020
Hudda
Hudda approved these changes Jul 27, 2020
Copy link
Member

@Hudda Hudda left a comment

Thanks @nishantwrp LGTM for codeowner files.

@Hudda Hudda removed their assignment Jul 27, 2020
Copy link
Member

@kevintab95 kevintab95 left a comment

LGTM for codeowner files. Thanks @nishantwrp!

@kevintab95 kevintab95 removed their assignment Jul 27, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jul 27, 2020

@sagangwee ptal!

Copy link
Contributor

@sagangwee sagangwee left a comment

@nishantwrp LGTM for codeowners.

@sagangwee sagangwee assigned nishantwrp and unassigned sagangwee Jul 27, 2020
@nishantwrp nishantwrp merged commit 1f60e00 into oppia:develop Jul 27, 2020
6 checks passed
@nishantwrp nishantwrp deleted the milestone-2 branch Jul 27, 2020
shavavo pushed a commit to shavavo/oppia that referenced this pull request Aug 20, 2020
…d remove remaining any types (oppia#10054)

* Refactor backend api services and remove remaining any types

* Remove testing code

* Revert unnecessary change

* Eslint check

* reviews

* reviews

* Update invalid_eslint_any_type.ts

* backend tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the AngularJs native variables which are typed as any
10 participants