-
Notifications
You must be signed in to change notification settings - Fork 183
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 #8305: failed to generate the changelog between HLC and Modular #8332
Conversation
tools/js-sdk-release-tools/src/mlc/apiVersion/apiVersionTypeExtractor.ts
Outdated
Show resolved
Hide resolved
process.exit(1); | ||
} | ||
// TODO: use a more concrete rule to find xxx.api.md | ||
return path.join(packageRoot, 'review', fileNames[fileNames.length - 1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does dev-tool run api-extractor
give us? is it the same as what openai/review folder get? Also I think it's a little risk to pick up the last one as sort order might be different vary on the system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the filenames are sorted here const fileNames = fs.readdirSync(reviewDir).sort();
. the current naming convension should give the same result.
but I just find I should make extract it from package name.
e.g. package is "@azure/arm-networkanalytics"
and the api view is named arm-networkanalytics.api.md
, which can be extracted from package name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the filenames are sorted here
const fileNames = fs.readdirSync(reviewDir).sort();
. the current naming convension should give the same result.but I just find I should make extract it from package name. e.g. package is "@azure/arm-networkanalytics" and the api view is named
arm-networkanalytics.api.md
, which can be extracted from package name.
just double check the dev-tool code,
the root export report name is <unscopedPackageName>.api.md
and ./subpath export will be <unscopedPackageName>-subpath .api.md
so this solution should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the investigation. I am a little doubt about the sort order actually, different os may have different behavior ? not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the string sort is keep consistent by javascript.
let apiMdFileLocal = path.join(packagePath, 'review', fs.readdirSync(path.join(packagePath, 'review'))[0]); | ||
const npmPackageRoot = path.join(packagePath, 'changelog-temp', 'package'); | ||
const apiMdFileNPM = getApiReviewPath(npmPackageRoot); | ||
const apiMdFileLocal = getApiReviewPath(packagePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe not a high priority for now but should we error out if it's comparing between RLC and HLC? or Modular api layer and HLC ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like all sdk type should error out in a general way. plan to start a new pr to do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave a TODO in code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
/check-enforcer override |
Generated CHANGELOG.md (2024-05-24):
HLC
Modular