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

Support customization process part 1 #1960

Merged
merged 10 commits into from
Aug 10, 2023

Conversation

MaryGao
Copy link
Member

@MaryGao MaryGao commented Aug 8, 2023

This is part of the pr - #1958

[Part 1] In this pr we would

  • Generate code in sources folder in customization case and in src folder in no customization

[Part 2] Below would be covered in my next pr:

  • Generate modular version for package.json and tsconfig.json
  • Generate metadata if relevant files are absent and not generate if existing

@MaryGao MaryGao changed the title Support customization process Support customization process part 1 Aug 9, 2023
@MaryGao MaryGao marked this pull request as ready for review August 9, 2023 05:16
@@ -0,0 +1,93 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mainly test to generate in sources/generated/src folder if exists this sources folder

Copy link
Member

@qiaozha qiaozha left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, just some minor comments.

let sourcesRoot = join(projectRoot, "src");
const customizationFolder = join(projectRoot, "sources");
if (await fsextra.pathExists(customizationFolder)) {
sourcesRoot = join(customizationFolder, "generated", "src");
Copy link
Member

Choose a reason for hiding this comment

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

Have we decided to use sources/generated/src folder?

Copy link
Member Author

@MaryGao MaryGao Aug 10, 2023

Choose a reason for hiding this comment

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

This is the way of current openai implementation and customization tool supported. Personally I think sources/generated and sources/generated/src would work me. The latter one could give us more flexibility if in future we'd like to add more metadata or testing files in sources folder.

Comment on lines +10 to +13
rootDir: string;
rlcSourcesDir: string;
modularSourcesDir?: string;
metadataDir: string;
Copy link
Member

Choose a reason for hiding this comment

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

is rootDIr not equals to metadataDir ?

Copy link
Member Author

@MaryGao MaryGao Aug 10, 2023

Choose a reason for hiding this comment

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

Generally rootDir and metadataDir are the same, I proposed a new option because I will use rootDir to calculate if this is in sdk repo and I think using metadata one would be wired.

Comment on lines +46 to +47
const options: RLCOptions = transformRLCOptions(emitterOptions, dpgContext);
dpgContext.rlcOptions = options;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const options: RLCOptions = transformRLCOptions(emitterOptions, dpgContext);
dpgContext.rlcOptions = options;
dpgContext.rlcOptions = transformRLCOptions(emitterOptions, dpgContext);

Is this not working ?

Copy link
Member

Choose a reason for hiding this comment

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

Just realized options is being used somewhere else ?

Copy link
Member

Choose a reason for hiding this comment

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

wonder if there's any place that will modify the options causing them to be inconsistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to refactor this code in my next pr.

@MaryGao MaryGao merged commit c84ceaf into Azure:main Aug 10, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants