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
158 changes: 158 additions & 0 deletions core/templates/domain/exploration/RuleInputTypeFactory.ts
@@ -0,0 +1,158 @@
// Copyright 2020 The Oppia Authors. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS-IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/**
* @fileoverview Factory to determine correct type of RuleInputs
*/

import { downgradeInjectable } from '@angular/upgrade/static';
import { Injectable } from '@angular/core';

import { IRuleInput } from 'domain/exploration/RuleObjectFactory';
import { INumberWithUnitsBackendDict } from
'domain/objects/NumberWithUnitsObjectFactory';
import { IFractionDict } from
'domain/objects/FractionObjectFactory.ts';
/* eslint-disable max-len */
import { INote } from 'extensions/interactions/MusicNotesInput/directives/music-notes-input-rules.service';
/* eslint-enable max-len */
import { IGraphBackendDict } from
'extensions/interactions/GraphInput/directives/graph-detail.service';

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

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

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

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

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

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

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.

if (this._isString(variable) || this._isNumber(variable)) {
return false;
}

return 'vertices' in variable;
}

_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') {
Copy link
Member

Choose a reason for hiding this comment

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

There are other datatypes as well, right? That can come here.
Can't you do something like if typeof variable[0] !== <dataType>? If not, I think this is fine.

Also, do the same check for all variables in the array, currently it is done only on the first element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aks681 Actually we don't have to check everything completely because we know variable will be of type IRuleInput which is (string | number | IFractionDict | INumberWithUnitsBackendDict | string[] | INote[] | number[] | IGraphBackendDict| string[][]). So we just have to differentiate among these types and checking the first element would do that, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto - add comments.

Copy link
Member

Choose a reason for hiding this comment

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

@nishantwrp As @ankita240796 said, add comments for all these. But, are we validating anywhere that IRuleInput will just be those and not anything else (apart from just mentioning its types)?

return false;
}

return 'readableNoteName' in variable[0];
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment, the field may be there in variable[0] but may not be in other elements, so make sure to check all elements.
Also, why aren't you verifying whether all the fields are there, why just this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reply as above.

}

return false;
}
Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

Copy link
Member

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

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

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

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

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

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

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

Choose a reason for hiding this comment

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

Ok, thanks for explaining.


_isNumberWithUnits(
variable: Object): variable is INumberWithUnitsBackendDict {
if (this._isString(variable) || this._isNumber(variable)) {
return false;
}

return 'fraction' in variable;
Copy link
Member

Choose a reason for hiding this comment

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

Ditto about checking all fields here and elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reply as above. We don't have to check everything. Moreover is it sure that in the entire codebase the numberWithUnits will have all the properties?. I saw that graph in some places had only the vertices property.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can add a high level comment to explain this logic -- if not with every type check. Or it would be a better idea to add some documentation around this on wiki as well just to avoid confusion.

}

_isFraction(variable: Object): variable is IFractionDict {
if (this._isString(variable) || this._isNumber(variable)) {
return false;
}

return 'numerator' in variable;
}

_isString(variable: Object): variable is string {
return typeof variable === 'string';
}

_isNumber(variable: Object): variable is number {
return typeof variable === 'number';
}

_isStringArray(variable: Object): variable is string[] {
return (Array.isArray(variable) && (
variable.length === 0 || typeof variable[0] === 'string'));
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here about only checking first element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reply as above.

}

_isStringArrayArray(variable: Object): variable is string[][] {
return (Array.isArray(variable) && (
variable.length === 0 || (Array.isArray(variable[0]) && (
variable[0].length === 0 || typeof variable[0][0] === 'string'))));
}

_isNumberArray(variable: Object): variable is number[] {
return (Array.isArray(variable) && (
variable.length === 0 || typeof variable[0] === 'number'));
}

graphInstance(variable: IRuleInput): IGraphBackendDict {
Copy link
Member

@aks681 aks681 May 27, 2020

Choose a reason for hiding this comment

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

Maybe rename to smtg like verifyGraphInstance since it is difficult to understand what the function does with the current name.

Also, what happens if it is not an instance, shouldn't there be a return null or maybe throw Error? and why is the same variable that is passed in, returned? Can't the calling function just use that variable if the check passes? This could just be an isGraphInstance function as well, right? That returns bool?

Ditto below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Okay, I think getGraphInstance seems more appropriate. WDYT?
  2. Yeah, right. done! (Thrown an error if it is not an instance).
  3. Same variable is returned because we are just using it to change the type of variable because want the ts compiler to know that variable is of type IGraphBackendDict (or other).
  4. It could be, but this way we can assure that only IRuleInput is passed as an argument.

Copy link
Member

@aks681 aks681 May 29, 2020

Choose a reason for hiding this comment

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

Ah ok, so you are basically type casting the variable from an IRuleInput to IGraphBackendDIct for ts compiler. Is that it? Add that in a comment for this function.
Knowing this, yeah, getGraphInstance sounds reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

if (this._isGraph(variable)) {
return variable;
}
}

notesInstance(variable: IRuleInput): INote[] {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto about adding comment.

if (this._isNoteArray(variable)) {
return variable;
}
}

numberWithUnitsInstance(variable: IRuleInput): INumberWithUnitsBackendDict {
if (this._isNumberWithUnits(variable)) {
return variable;
}
}

fractionInstance(variable: IRuleInput): IFractionDict {
if (this._isFraction(variable)) {
return variable;
}
}

stringInstance(variable: IRuleInput): string {
if (this._isString(variable)) {
return variable;
}
}

numberInstance(variable: IRuleInput): number {
if (this._isNumber(variable)) {
return variable;
}
}

stringArrayInstance(variable: IRuleInput): string[] {
if (this._isStringArray(variable)) {
return variable;
}
}

stringArrayArrayInstance(variable: IRuleInput): string[][] {
if (this._isStringArrayArray(variable)) {
return variable;
}
}

numberArrayInstance(variable: IRuleInput): number[] {
if (this._isNumberArray(variable)) {
return variable;
}
}
}

angular.module('oppia').factory('RuleInputTypeFactory',
downgradeInjectable(RuleInputTypeFactory));