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: Add type definitions in some object factories #9345

Merged
merged 6 commits into from May 28, 2020
Merged

Fix part of #7176: Add type definitions in some object factories #9345

merged 6 commits into from May 28, 2020

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented May 22, 2020

Overview

  1. This PR fixes or fixes part of Add types for Object Factories  #7176 .
  2. This PR does the following: Add type definitions in some object factories

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 22, 2020

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

@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #9345 into develop will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #9345      +/-   ##
===========================================
- Coverage    54.07%   54.06%   -0.00%     
===========================================
  Files          878      878              
  Lines        36394    36392       -2     
  Branches      4357     4357              
===========================================
- Hits         19677    19675       -2     
  Misses       15722    15722              
  Partials       995      995              
Flag Coverage Δ
#frontend 54.06% <100.00%> (-<0.01%) ⬇️
Impacted Files Coverage Δ
...lates/domain/classifier/ClassifierObjectFactory.ts 100.00% <ø> (ø)
...ack_message/FeedbackMessageSummaryObjectFactory.ts 100.00% <ø> (ø)
...dback_thread/FeedbackThreadSummaryObjectFactory.ts 91.43% <ø> (ø)
...hboard/LearnerDashboardActivityIdsObjectFactory.ts 96.36% <ø> (ø)
...plates/domain/topic/StoryReferenceObjectFactory.ts 100.00% <ø> (ø)
...ain/exploration/WrittenTranslationObjectFactory.ts 100.00% <100.00%> (ø)
...in/exploration/WrittenTranslationsObjectFactory.ts 98.61% <100.00%> (ø)
...ain/feedback_message/ThreadMessageObjectFactory.ts 96.67% <100.00%> (ø)
...ain/feedback_thread/FeedbackThreadObjectFactory.ts 96.97% <100.00%> (ø)
...mplates/domain/story/StoryContentsObjectFactory.ts 84.98% <100.00%> (ø)
... and 10 more

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! Two comments, also please reference #7165 and #7176 in your PRs.

export interface ITranslationBackendDict {
html: string;
'needs_update': boolean;
}
Copy link
Member

@vojtechjelinek vojtechjelinek May 22, 2020

Choose a reason for hiding this comment

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

Maybe use the quotes consistently? So if there is at least one quoted item quote all the items. Ditto for other places.

Copy link
Contributor Author

@nishantwrp nishantwrp May 23, 2020

Choose a reason for hiding this comment

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

done

import { IStoryReferenceBackendDict, StoryReference,
StoryReferenceObjectFactory } from
Copy link
Member

@vojtechjelinek vojtechjelinek May 22, 2020

Choose a reason for hiding this comment

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

Maybe:

Suggested change
import { IStoryReferenceBackendDict, StoryReference,
StoryReferenceObjectFactory } from
import {
IStoryReferenceBackendDict,
StoryReference,
StoryReferenceObjectFactory
} from 'domain/topic/StoryReferenceObjectFactory';

Copy link
Contributor Author

@nishantwrp nishantwrp May 23, 2020

Choose a reason for hiding this comment

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

done

@kevinlee12
Copy link
Contributor

kevinlee12 commented May 23, 2020

@vojtechjelinek please update the assignee list when you are done with your part, thanks!

@nishantwrp nishantwrp changed the title Add type definitions in some object factories Fix part of #7165: Add type definitions in some object factories May 23, 2020
@nishantwrp nishantwrp changed the title Fix part of #7165: Add type definitions in some object factories Fix part of #7176: Add type definitions in some object factories May 23, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 23, 2020

@vojtechjelinek PTAL!

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

LGTM!

Copy link
Member

@kevintab95 kevintab95 left a comment

Thanks @nishantwrp! LGTM with one minor comment. PTAL!

@@ -44,7 +44,9 @@ describe('Story contents object factory', () => {
destination_node_ids: ['node_2'],
outline: 'Outline',
exploration_id: null,
outline_is_finalized: false
outline_is_finalized: false,
thumbnail_bg_color: 'white',
Copy link
Member

@kevintab95 kevintab95 May 24, 2020

Choose a reason for hiding this comment

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

Can you make this hex and make sure it is the correct value for this domain object (see constants.ts)? (Do this in the topic, subtopic, and story node specs too.)

Copy link
Contributor Author

@nishantwrp nishantwrp May 24, 2020

Choose a reason for hiding this comment

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

done

@kevintab95 kevintab95 removed their assignment May 24, 2020
Copy link
Contributor

@ankita240796 ankita240796 left a comment

LGTM from code-owner's perspective, Thanks @nishantwrp!

@ankita240796 ankita240796 removed their assignment May 25, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 25, 2020

@prasanna08 @aks681 @nithusha21 @marianazangrossi PTAL!

Copy link
Contributor

@prasanna08 prasanna08 left a comment

Code owner, LGTM!

@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 26, 2020

@aks681 @marianazangrossi @nithusha21 PTAL!

Copy link
Contributor

@marianadasilvadev marianadasilvadev left a comment

LGTM, just a minor nit, thanks @nishantwrp

@@ -36,7 +36,41 @@ describe('State classifier mapping service', () => {
mappingService.init({
stateName1: {
algorithm_id: 'TestClassifier',
classifier_data: {},
classifier_data: {
Copy link
Contributor

@marianadasilvadev marianadasilvadev 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 you can keep this object in a variable and use this variable in other places instead of asserting the hardcoded object.

Copy link
Contributor Author

@nishantwrp nishantwrp May 27, 2020

Choose a reason for hiding this comment

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

done

@oppiabot
Copy link

oppiabot bot commented May 27, 2020

Hi @nishantwrp. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

@nishantwrp
Copy link
Contributor Author

nishantwrp commented May 27, 2020

@nithusha21 @aks681 Ping!

aks681
aks681 approved these changes May 27, 2020
Copy link
Member

@aks681 aks681 left a comment

lgtm as codeowner

@aks681 aks681 removed their assignment May 27, 2020
Copy link
Member

@nithusha21 nithusha21 left a comment

LGTM for codeowner files!

@nithusha21
Copy link
Member

nithusha21 commented May 28, 2020

Please note failing travis tests!

@nithusha21 nithusha21 assigned nishantwrp and unassigned nithusha21 May 28, 2020
@kevinlee12 kevinlee12 merged commit b6ba041 into oppia:develop May 28, 2020
18 of 19 checks passed
@nishantwrp nishantwrp deleted the any-type-1 branch May 28, 2020
shavavo pushed a commit to shavavo/oppia that referenced this pull request Aug 20, 2020
…oppia#9345)

* Add type definitions in some object factories

* Refactor Code

* Refactor Code

* Use hexcode

* Make classifier_data a variable
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

10 participants