-
-
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 - III #7214
Conversation
This reverts commit 13424bf.
FeedbackThreadSummaryObjectFactory
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.
Looks great for the most part, just two comments, PTAL!
core/templates/dev/head/domain/exploration/AnswerStatsObjectFactory.ts
Outdated
Show resolved
Hide resolved
core/templates/dev/head/services/StateTopAnswersStatsServiceSpec.ts
Outdated
Show resolved
Hide resolved
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.
lgtm!
@seanlip @vojtechjelinek ptal! |
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.
Hi @YashJipkate, took a pass -- PTAL. Thanks!
import { downgradeInjectable } from '@angular/upgrade/static'; | ||
|
||
export interface IAnswerStatsBackendDict { | ||
// TODO(YashJipkate): Replace 'any' with the exact type. This has been kept as |
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.
Please remember to update the list in #7176 -- we've discussed that in previous PRs and I shouldn't need to send a reminder about this.
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.
Done!
// TODO(YashJipkate): Replace 'any' with the exact type. This has been kept as | ||
// 'any' because 'schema' is a complex dict requiring very careful | ||
// backtracking. | ||
// https://github.com/oppia/oppia/issues/7165 |
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.
Ditto. Update #7165 with a checkbox list. Do this for past PRs as well.
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.
Done!
@@ -34,6 +42,27 @@ require('App.ts'); | |||
require('pages/exploration-editor-page/services/exploration-states.service.ts'); | |||
require('services/StateTopAnswersStatsService.ts'); | |||
|
|||
class MockAnswerStats { | |||
answer: any; |
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.
Where are the comments for this?
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.
Actually, this is in AngularJS, therefore I did not add comments.
I'll add them too.
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.
Ah -- I think AngularJS/Angular is the same as far as TypeScript is concerned, so yep, please add the necessary todos/comments for any code you touch.
@@ -26,6 +26,15 @@ describe('Rule spec services', function() { | |||
angular.mock.module('oppia'); | |||
}); | |||
|
|||
beforeEach(angular.mock.module('oppia', function($provide) { | |||
$provide.value('CodeNormalizerService', new CodeNormalizerService()); | |||
// This service is not mocked by using its actual class instance since the |
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 don't understand what this comment is trying to say, sorry. Can you elaborate more please?
In particular, what exactly do you mean by "iterative way"?
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.
Actually, the rulesSpec uses a loop to go through the services and initializes them in an 'iterative' way and therefore needs to have consistency and thus have to be all initialized the same way.
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.
That's a much better explanation (especially re the consistency and the need to initialize them in the same way). Please say that in the comment :)
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.
LGTM. @vojtechjelinek, PTAL, then this can be merged.
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.
Thanks! Just on speed-related comment!
@seanlip @kevinlee12 @vojtechjelinek The tests now pass. PTAL! |
Explanation
This PR upgrades:
Checklist
python scripts/pre_commit_linter.py
andbash scripts/run_frontend_tests.sh
.