Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

chore: copy Constructor in 'shared' lib #1501

Merged
cwhitten merged 50 commits into from
Nov 18, 2019
Merged

chore: copy Constructor in 'shared' lib #1501

cwhitten merged 50 commits into from
Nov 18, 2019

Conversation

@yeze322
Copy link
Contributor

@yeze322 yeze322 commented Nov 5, 2019 •
edited by a-b-r-o-w-n
Loading

Description

Implement the copy constructor in dialogFactory to enable action deep clone.

Obi types need sepcial treatment

  1. Nested
    • IfCondition
    • SwitchCondition
    • Foreach
    • Foreachpage
    • EditActions
  2. With LG templates
    • SendActivity
    • All kinds of *Input (TextInput, ChoiceInput, .etc.)

Task Item

refs #1193, #1191

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code refactor (non-breaking change which improve code quality, clean up, add tests, etc)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Doc update (document update)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have functionally tested my change

Screenshots

Please include screenshots or gifs if your PR include UX changes.

@yeze322 yeze322 mentioned this pull request Nov 5, 2019
Merged
9 tasks
@yeze322
Copy link
Contributor Author

yeze322 commented Nov 7, 2019

@a-b-r-o-w-n @boydc2014 please review the current pattern we use.

a-b-r-o-w-n
a-b-r-o-w-n suggested changes Nov 8, 2019
View reviewed changes
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment

Choose a reason for hiding this comment

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

Overall, I think this is a solid design. Tactically, I would like to see some more error handling.

a-b-r-o-w-n
a-b-r-o-w-n suggested changes Nov 13, 2019
View reviewed changes
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment

Choose a reason for hiding this comment

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

Just some test feedback

@yeze322 yeze322 requested a review from a-b-r-o-w-n November 14, 2019 04:08
a-b-r-o-w-n
a-b-r-o-w-n previously approved these changes Nov 14, 2019
View reviewed changes
boydc2014
boydc2014 reviewed Nov 15, 2019
View reviewed changes
boydc2014
boydc2014 reviewed Nov 15, 2019
View reviewed changes
Composer/packages/lib/shared/src/copyUtils/copyInputDialog.ts Outdated
copy.prompt = await externalApi.copyLgTemplate(input.prompt, nodeId);
}

if (input.unrecognizedPrompt) {
Copy link
Contributor

@boydc2014 boydc2014 Nov 15, 2019

Choose a reason for hiding this comment

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

Similar things like this "unrecoginzedPrompt". Not sure this is required or not, just want to make sure we leverage the type checking as much as possible.

Because we control the "constructor", we could be able to enforce more strict type (less optional field). Not for just reducing 1 or 2 line codes, more importantly, have the type enforced at first place, will reduce the possible state in our system, means we will reduce the overall complexity of the system.

@boydc2014
Copy link
Contributor

boydc2014 commented Nov 15, 2019

Looks good, i leave a few comments about if we can remove some checking, by enforcing the structure more strictly, which would help reduce the overall complexity. It might not be the major focus of this PR, but it's a good direction to follow.

@yeze322
Copy link
Contributor Author

yeze322 commented Nov 15, 2019

I got Dong's point, we have declared some fields as required in sdk but during construction we missed the default values for some required fields. Though the 'copy constructor' should respect the original input to copy the data as it was, it's still a 'constructor' which means that it should generate default values for those required fields.

@yeze322
Copy link
Contributor Author

yeze322 commented Nov 15, 2019

However, I know a strange behavior which makes me cannot assgin a default value for those propmpt fields:
empty-string-in-activity
@boydc2014

@yeze322 yeze322 force-pushed the zeye/copy-constructor branch from 8c50877 to 2d1c076 Compare November 15, 2019 07:55
@cwhitten
Copy link
Member

cwhitten commented Nov 16, 2019

I got Dong's point, we have declared some fields as required in sdk but during construction we missed the default values for some required fields. Though the 'copy constructor' should respect the original input to copy the data as it was, it's still a 'constructor' which means that it should generate default values for those required fields.

assignDefaults exported from dialogFactory.ts should handle assigning any/all default values as defined off of the SDK.

@yeze322
Copy link
Contributor Author

yeze322 commented Nov 18, 2019 •
edited
Loading

@cwhitten I've added default fields for SendActivity and *Input types. (initialDialogShape in dialogFactory)

@yeze322 yeze322 force-pushed the zeye/copy-constructor branch from 909ed32 to 126eb5a Compare November 18, 2019 10:37
@cwhitten cwhitten merged commit e18d0ad into master Nov 18, 2019
@cwhitten cwhitten deleted the zeye/copy-constructor branch November 18, 2019 18:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@boydc2014 boydc2014 boydc2014 left review comments

@cwhitten cwhitten cwhitten approved these changes

@a-b-r-o-w-n a-b-r-o-w-n a-b-r-o-w-n left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yeze322 @boydc2014 @cwhitten @a-b-r-o-w-n