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

Conversation

@zhixzhan
Copy link
Contributor

Description

use lg parser api do template add/update/delete, instead of manually stitching text file.

Task Item

fix #1211

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)

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
Copy link
Contributor

yeze322 commented Oct 30, 2019

This is great. We can hide lg logic from store actions! Then we can extend those lg handlers and add apis like 'copyTemplate'

@zhixzhan
Copy link
Contributor Author

This is great. We can hide lg logic from store actions! Then we can extend those lg handlers and add apis like 'copyTemplate'

Yes, I'm trying new lg parser, and collecting feedback to lg team. if we want add some api or reporting issue, we can note here.
https://microsoft-my.sharepoint.com/:o:/p/zhixzhan/EolTpJpG1L5Gk_lI_Uzb9okBhVuBVgv6jROMSlLyl0kHXg?e=NbQerU

}

export function updateTemplate(content: string, templateName: string, { Name, Parameters, Body }: Template): string {
const resource = LGParser.parse(content);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the store should hold a bunch of resources? so we don't have to parse it every time?

Copy link
Contributor Author

@zhixzhan zhixzhan Oct 31, 2019

Choose a reason for hiding this comment

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

lgUtil do not have state, or we pass a parsed content to it ?

content: string ---> content: LGResource

Copy link
Contributor Author

@zhixzhan zhixzhan Oct 31, 2019

Choose a reason for hiding this comment

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

thought again, this way would increase the cost of caller.
how about cache it at lgUtil, then calculate hash diff determine re-parse it or not.

const localParsedLG = { id, contentHash, LGResource };

export function updateTemplate(content: string, templateName: string, { Name, Parameters, Body }: Template): string {
  const contentHash = hash(content);
  let resource;
  if( contentHash in localParsedLG ) {
      resource =  localParsedLG.get(contentHash)
  } else {
      resource = LGParser.parse(content);
     //update localParsedLG
  }
  ...
}

maybe this method should implmented withing lg parser

@zhixzhan zhixzhan marked this pull request as ready for review November 4, 2019 05:52
@yeze322
Copy link
Contributor

yeze322 commented Nov 5, 2019

Feature request: can we have a copyTemplate api in lgUtils?

const copyTemplate: (content: string, fromTemplateName: string, toTemplateName: string) => string;

It's like a synthesized api consists of addTemplate and getTemplate

@zhixzhan
Copy link
Contributor Author

zhixzhan commented Nov 6, 2019

Feature request: can we have a copyTemplate api in lgUtils?

const copyTemplate: (content: string, fromTemplateName: string, toTemplateName: string) => string;

It's like a synthesized api consists of addTemplate and getTemplate

Sure~, will add it. one more thing to confirm.
if toTemplateName already exist, you prefer:

  1. do nothing, or return an error code -1
  2. add with number postfix like ${toTemplateName}.Copy1

if toTemplateName is not defined, I prefer auto generate a name like ${fromTemplateName}.Copy1
it will align with current all-up-view logic.

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

zhixzhan commented Nov 6, 2019

This PR origin lost fork information, can't auto update commit anymore, move to #1512

@zhixzhan zhixzhan closed this Nov 6, 2019
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.

3 participants