-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Upgrade more services to Angular 8 #7145
Changes from all commits
13424bf
5c8bec3
3b51282
3ae97e8
7f22747
ffa8a7e
952d5c0
41987d5
9990fe5
027340d
cc0ec34
906fd76
5b56005
1502ad2
f3a0997
5e96b0f
6f906ef
feb8a4c
56f61f0
64f9fea
69681da
04d7091
727a6bf
abb470f
d8e573b
6fa6c50
0ca2eae
5c75b38
0e557bf
dff56fd
e26e464
aa5133d
951982b
de7d2ad
ce5cec8
b5c2ef9
b6f243f
3df1a3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,24 +17,39 @@ | |
* Classification Result domain objects. | ||
*/ | ||
|
||
var oppia = require('AppInit.ts').module; | ||
import { downgradeInjectable } from '@angular/upgrade/static'; | ||
import { Injectable } from '@angular/core'; | ||
|
||
export class AnswerClassificationResult { | ||
outcome: any; | ||
answerGroupIndex: number; | ||
ruleIndex: number; | ||
classificationCategorization: string; | ||
|
||
oppia.factory('AnswerClassificationResultObjectFactory', [function() { | ||
var AnswerClassificationResult = function( | ||
outcome, answerGroupIndex, ruleIndex, classificationCategorization) { | ||
constructor( | ||
outcome: any, answerGroupIndex: number, ruleIndex: number, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't outcome be an Outcome domain object? I thought we were veering away from using "any", in general. Please try to replace "any" with something more specific in all other files. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The complete elimination of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, let's file an issue. But I want to dig deeper into this particular case. What would 'outcome' end up being typed as? Let's at least establish a pattern for getting the right types for objects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed #7165. I think if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Try doing that here (in this PR), just for one case. Then link to this in the issue you filed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, here's what I suggest then. Can we type everything, except for things that can't be typed because they are still in AngularJS? I.e., instead of "complex objects" as the criterion, it should be specifically "objects that are still in AngularJS"? In general, typing things should not be hard. The specific issue you mention is a fair one but what we should be doing, e.g., is to update the typing for "outcomes" throughout the codebase in the same PR that migrates OutcomeObjectFactory to Angular. This is fine to do on a best-effort basis and we can do a final round of cleanup after everything is migrated (per #7165), but we shouldn't wilfully ignore it. Does this sound OK? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that sounds pretty good and I am currently also trying to eliminate as much But the problem of determining the correct type of some complex types (like arrays) and types of those with no spec files from which I could determine the types is super difficult, especially because we use an elaborate network of services so the parameters to a service in a non-spec file is always a reference to some other service and the loop goes on, sometimes even reaching the backend. This would take a great amount of time. That's why I came up with the 'complex' basis. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Understood. Thanks @YashJipkate! I do get the point about complex types (though arrays of primitives ought to be doable...) and I am fine with leaving actually-complex types till a final pass. I just think that currently the bar of "complex" == "not primitive" is set a little too low. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that can be definitely done and I am trying to do as much as possible! Please let me know if I missed out something. |
||
classificationCategorization: string) { | ||
this.outcome = outcome; | ||
this.answerGroupIndex = answerGroupIndex; | ||
this.ruleIndex = ruleIndex; | ||
this.classificationCategorization = classificationCategorization; | ||
}; | ||
} | ||
} | ||
|
||
// TODO (ankita240796) Remove the bracket notation once Angular2 gets in. | ||
/* eslint-disable dot-notation */ | ||
AnswerClassificationResult['createNew'] = function( | ||
/* eslint-enable dot-notation */ | ||
outcome, answerGroupIndex, ruleIndex, classificationCategorization) { | ||
@Injectable({ | ||
providedIn: 'root' | ||
}) | ||
export class AnswerClassificationResultObjectFactory { | ||
createNew( | ||
outcome: any, answerGroupIndex: number, ruleIndex: number, | ||
classificationCategorization: string) { | ||
return new AnswerClassificationResult( | ||
outcome, answerGroupIndex, ruleIndex, classificationCategorization); | ||
}; | ||
return AnswerClassificationResult; | ||
}]); | ||
} | ||
} | ||
|
||
var oppia = require('AppInit.ts').module; | ||
|
||
oppia.factory( | ||
'AnswerClassificationResultObjectFactory', | ||
downgradeInjectable(AnswerClassificationResultObjectFactory)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why these need to be added here. They don't seem to be used in the test?
There should probably be documentation somewhere to explain -- either here if it's specific to this, or in our wiki if it's a general principle to follow. In any case I think it would be nice to add a comment above line 25 explaining why lines 26-29 are needed. Do the same in all other similar files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not specific to this file, this is the case in many files. What's happening is that since we do not use isolated tests, these spec files depended upon the upgraded services and therefore angular mock could not interpret them.
This would be more suited to the wiki than a comment. I'll add this to the Angular wiki.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that would be removed after the full migration to Angular 8?
If so, we should add a TODO to remove them in the future, like Vojta did for some of the webpack requires. I realize that that might be repetitive, but we should do that anyway to make clear that this isn't a long-term practice to be followed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The statements can be removed once the corresponding non-spec file is upgraded.
Added the TODOs!