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 part of #7176: Remove any from RuleObjectFactory and introduce TypeChangeService #9365

Merged
merged 8 commits into from Jun 3, 2020
Merged

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented May 26, 2020

Overview

  1. This PR fixes or fixes part Add types for Object Factories  #7176
  2. This PR does the following: Remove any from RuleObjectFactory and introduce first TypeFactory

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 May 26, 2020

Hi, @nishantwrp. This pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Please add this 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!

@oppiabot
Copy link

oppiabot bot commented May 26, 2020

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

_isNoteArray(variable: Object): variable is INote[] {
if (Array.isArray(variable)) {
if (variable.length === 0) {
return true;
}

if (typeof variable[0] === 'string' || typeof variable[0] === 'number') {
return false;
}

return 'readableNoteName' in variable[0];
}

return false;
}
Copy link
Contributor Author

@nishantwrp nishantwrp May 26, 2020

Choose a reason for hiding this comment

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

Now, in these functions we had two options

  • simply write return true;
  • Actually check the type of the variable

Currently I've followed the second point.
Any can be used since we are going to use this factory only for changing the type of IRuleInput to the actual type.

Which one do you guys prefer?

Copy link
Contributor Author

@nishantwrp nishantwrp May 26, 2020

Choose a reason for hiding this comment

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

Copy link
Member

@vojtechjelinek vojtechjelinek May 26, 2020

Choose a reason for hiding this comment

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

I'm not sure what is the difference here, in one case we would be using just variable is INote[] and in the second there is the actual code?

Copy link
Contributor Author

@nishantwrp nishantwrp May 26, 2020

Choose a reason for hiding this comment

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

The first point means something like this

_isNoteArray(variable: Object): variable is INote[] {
    return true;
  }

Copy link
Member

@vojtechjelinek vojtechjelinek May 27, 2020

Choose a reason for hiding this comment

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

I think that the second option is fine, thanks!

Copy link
Contributor Author

@nishantwrp nishantwrp May 29, 2020

Choose a reason for hiding this comment

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

Also, I was thinking we could have a general TypeChangeService instead of TypeFactories if we agree on the above point.

Copy link
Member

@vojtechjelinek vojtechjelinek May 29, 2020

Choose a reason for hiding this comment

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

I'm fine with using the first appraoch if you're not able to do it with the second.

Copy link
Contributor

@ankita240796 ankita240796 May 30, 2020

Choose a reason for hiding this comment

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

Hmm, due to the failure of e2e tests, I think we would have to go with the first option. Because somewhere in the codebase it is happening that the variable should be of some type but is currently null or at that time has no properties.

So, are we planning to fix this later or is there any other issue with fixing this?

Copy link
Contributor Author

@nishantwrp nishantwrp May 30, 2020

Choose a reason for hiding this comment

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

No I think it doesn't need to be fixed. It's something like suppose inputs is of type IRuleInputs and then inputs.x is of type IRuleInput and is currently empty. And we are doing something like

inputs.x.vertices = [2,2];

It will throw a ts error that vertices don't exist on type IRuleInput.

Now if we use our type factory it will throw an error as well because vertices don't exist on inputs.x.

But with the first method, we can just assume that it is of type IGraphDict.

Copy link
Contributor

@ankita240796 ankita240796 May 31, 2020

Choose a reason for hiding this comment

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

Ok, thanks for explaining.

@nishantwrp nishantwrp changed the title Remove any from RuleObjectFactory and introduce first TypeFactory Fix part of #7176: Remove any from RuleObjectFactory and introduce first TypeFactory May 26, 2020
Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! Few comments and questions.

/* eslint-disable max-len */
import { INote } from 'extensions/interactions/MusicNotesInput/directives/music-notes-input-rules.service';
import { IGraphBackendDict } from 'extensions/interactions/GraphInput/directives/graph-detail.service';
/* eslint-enable max-len */
Copy link
Member

@vojtechjelinek vojtechjelinek May 26, 2020

Choose a reason for hiding this comment

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

can these be split into multiple lines?

Copy link
Contributor Author

@nishantwrp nishantwrp May 26, 2020

Choose a reason for hiding this comment

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

'extensions/interactions/MusicNotesInput/directives/music-notes-input-rules.service' is more than 80 chars. I don't know how can it be split.

providedIn: 'root'
})
export class RuleInputTypeFactory {
_isGraph(variable: Object): variable is IGraphBackendDict {
Copy link
Member

@vojtechjelinek vojtechjelinek May 26, 2020

Choose a reason for hiding this comment

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

Could you please explain how variable is IGraphBackendDict works?

Copy link
Contributor Author

@nishantwrp nishantwrp May 26, 2020

Choose a reason for hiding this comment

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

It tells the ts compiler that the variable is of type IGraphBackendDict

Copy link
Member

@vojtechjelinek vojtechjelinek May 26, 2020

Choose a reason for hiding this comment

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

Hmm, but how is it different from variable: IGraphBackendDict?

Copy link
Contributor Author

@nishantwrp nishantwrp May 26, 2020

Choose a reason for hiding this comment

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

Case 1

func1(a: Object): GraphDict {
...
}

It means that func1 returns a object whose type is GraphDict

Case 2

func1(a: Object): a is GraphDict {
...
}

It means that func1 returns a boolean and if thats true it means a is of type GraphDict

Copy link
Member

@vojtechjelinek vojtechjelinek May 27, 2020

Choose a reason for hiding this comment

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

Ok, thanks

Copy link
Contributor

@ankita240796 ankita240796 May 28, 2020

Choose a reason for hiding this comment

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

I would suggest to add comments for all this. In general, wherever you think a reviewer might have a question, add comments in advance.

// exact type needs to be found for it, Once that is found the return type
// can also be typed.
convertUnitDictToList(unitDict: any): any[] {
convertUnitDictToList(unitDict: any): IUnit[] {
Copy link
Member

@vojtechjelinek vojtechjelinek May 26, 2020

Choose a reason for hiding this comment

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

any is still used here

Copy link
Contributor Author

@nishantwrp nishantwrp May 26, 2020

Choose a reason for hiding this comment

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

Missed it! Done!

@vojtechjelinek
Copy link
Member

vojtechjelinek commented May 26, 2020

@kevintab95 @aks681 @marianazangrossi Code owner review, please.

Copy link
Contributor

@marianadasilvadev marianadasilvadev left a comment

LGTM @nishantwrp, just a few minor nits

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #9365 into develop will increase coverage by 0.26%.
The diff coverage is 74.07%.

@@             Coverage Diff             @@
##           develop    #9365      +/-   ##
===========================================
+ Coverage    54.07%   54.33%   +0.26%     
===========================================
  Files          878      882       +4     
  Lines        36394    36705     +311     
  Branches      4357     4411      +54     
===========================================
+ Hits         19677    19941     +264     
- Misses       15722    15726       +4     
- Partials       995     1038      +43     
Flag Coverage Δ
#frontend 54.33% <74.07%> (+0.26%) ⬆️
Impacted Files Coverage Δ
...ctives/math-expression-input-validation.service.ts 41.67% <25.00%> (+0.76%) ⬆️
...put/directives/numeric-input-validation.service.ts 75.28% <30.00%> (-0.58%) ⬇️
...Input/directives/graph-input-validation.service.ts 82.93% <40.00%> (-1.28%) ⬇️
...ectives/item-selection-input-validation.service.ts 65.22% <40.00%> (-0.34%) ⬇️
...ves/drag-and-drop-sort-input-validation.service.ts 89.47% <70.00%> (-0.64%) ⬇️
core/templates/services/type-change.service.ts 82.00% <82.00%> (ø)
.../templates/domain/exploration/RuleObjectFactory.ts 100.00% <100.00%> (ø)
...tes/domain/objects/NumberWithUnitsObjectFactory.ts 91.23% <100.00%> (ø)
...ore/templates/domain/objects/UnitsObjectFactory.ts 100.00% <100.00%> (ø)
...ions/GraphInput/directives/graph-detail.service.ts 21.88% <100.00%> (ø)
... and 72 more

@nishantwrp nishantwrp changed the title Fix part of #7176: Remove any from RuleObjectFactory and introduce first TypeFactory Fix part of #7176: Remove any from RuleObjectFactory and introduce TypeChangeService Jun 1, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jun 1, 2020

@aks681 @DubeySandeep @kevintab95 PTAL!

@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jun 1, 2020

I've refactored to create a TypeChangeService as discussed offline with @vojtechjelinek.

Copy link
Member

@kevintab95 kevintab95 left a comment

Code-owner files LGTM, thanks @nishantwrp!

@kevintab95 kevintab95 removed their assignment Jun 1, 2020
@nishantwrp nishantwrp requested a review from aks681 Jun 2, 2020
Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

LGTM!

@vojtechjelinek vojtechjelinek removed their assignment Jun 2, 2020
@vojtechjelinek
Copy link
Member

vojtechjelinek commented Jun 2, 2020

@DubeySandeep @aks681 PTAL

aks681
aks681 approved these changes Jun 2, 2020
Copy link
Member

@aks681 aks681 left a comment

Lgtm as codeowner.

@aks681 aks681 removed their assignment Jun 2, 2020
Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

@nishantwrp wants to make some changes in this PR.

@nishantwrp nishantwrp requested a review from vojtechjelinek Jun 3, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jun 3, 2020

@vojtechjelinek done ptal!

@nishantwrp nishantwrp removed the request for review from DubeySandeep Jun 3, 2020
Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

LGTM!

@bansalnitish bansalnitish merged commit 66597a9 into oppia:develop Jun 3, 2020
15 checks passed
@nishantwrp nishantwrp deleted the any-type-3 branch Jun 3, 2020
shavavo pushed a commit to shavavo/oppia that referenced this pull request Aug 20, 2020
…ce TypeChangeService (oppia#9365)

* Remove any from RuleObjectFactory and introduce first TypeFactory

* Reviews

* indentify -> identify

* Throw error

* Change function name

* refactor to create a type change service

* remove type change service
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants