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

Conversation

@zhixzhan
Copy link
Contributor

@zhixzhan zhixzhan commented Nov 6, 2019

Description

  1. Leverage lg parser api do template add/update/delete, instead of manually stitching text file.
  2. Add more lg template operation methods in lgUtil

Task Item

fix #1211
fix #1213

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)

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.

@zhixzhan
Copy link
Contributor Author

zhixzhan commented Nov 6, 2019

This pr #1349 lost fork information, move to here

@zhixzhan zhixzhan mentioned this pull request Nov 6, 2019
6 tasks
@zhixzhan
Copy link
Contributor Author

zhixzhan commented Nov 6, 2019

@yeze322 Added couple of methods:

1. copyTemplate(content: string, fromTemplateName: string, toTemplateName?: string): string 

2. removeTemplates(content: string, templateNames: string[]): string

// if Name exist, throw error.
3. addTemplate(content: string, { Name, Parameters, Body }: Template): string

// if Name exist, add it anyway, with name like `${Name}1` `${Name}2`
4. addTemplateAnyway(
  content: string,
  { Name = 'TemplateName', Parameters = [], Body = '-TemplateBody' }: Template
): string

and applied to visual editor ObiEditor.tsx, please review~

@yeze322
Copy link
Contributor

yeze322 commented Nov 6, 2019

copyTemplate is good enough for visual editor to use!

@yeze322
Copy link
Contributor

yeze322 commented Nov 6, 2019

Changes on removeTemplates is related to #1213 , link it.

@yeze322
Copy link
Contributor

yeze322 commented Nov 11, 2019

@boydc2014

boydc2014
boydc2014 previously approved these changes Nov 12, 2019
@boydc2014
Copy link
Contributor

@yeze322 Added couple of methods:

1. copyTemplate(content: string, fromTemplateName: string, toTemplateName?: string): string 

2. removeTemplates(content: string, templateNames: string[]): string

// if Name exist, throw error.
3. addTemplate(content: string, { Name, Parameters, Body }: Template): string

// if Name exist, add it anyway, with name like `${Name}1` `${Name}2`
4. addTemplateAnyway(
  content: string,
  { Name = 'TemplateName', Parameters = [], Body = '-TemplateBody' }: Template
): string

and applied to visual editor ObiEditor.tsx, please review~

Looks good, and can you add those signature into comments of code at somewhere?

@zhixzhan
Copy link
Contributor Author

@yeze322 Added couple of methods:

1. copyTemplate(content: string, fromTemplateName: string, toTemplateName?: string): string 

2. removeTemplates(content: string, templateNames: string[]): string

// if Name exist, throw error.
3. addTemplate(content: string, { Name, Parameters, Body }: Template): string

// if Name exist, add it anyway, with name like `${Name}1` `${Name}2`
4. addTemplateAnyway(
  content: string,
  { Name = 'TemplateName', Parameters = [], Body = '-TemplateBody' }: Template
): string

and applied to visual editor ObiEditor.tsx, please review~

Looks good, and can you add those signature into comments of code at somewhere?

Yes, added to lgUtils.ts

@zhixzhan
Copy link
Contributor Author

@boydc2014 can it be merged?

@boydc2014
Copy link
Contributor

Looks good.

@boydc2014 boydc2014 merged commit 3a9f5ed into microsoft:master Nov 12, 2019
@boydc2014
Copy link
Contributor

FYI, @cwhitten , I merged this, since it's most additive and it's a blocker for @yeze322's work of LG copy\delete.

@yeze322
Copy link
Contributor

yeze322 commented Nov 12, 2019

Thanks to everyone!

@zhixzhan zhixzhan deleted the lg-editor branch December 2, 2019 02:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants