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

generate changelog notes #71

Closed
wants to merge 11 commits into from
Closed

generate changelog notes #71

wants to merge 11 commits into from

Conversation

jshawl
Copy link
Member

@jshawl jshawl commented Feb 23, 2024

This PR adds an action step to generate the changelog notes for the latest JS SDK release:

changelog

For this demo, I added a step to the main.yml workflow to test it but moved it to publish.yml in 42ca4b3

Copy link
Member

@wsbrunson wsbrunson left a comment

Choose a reason for hiding this comment

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

nothing blocking, this is looking really good

Comment on lines +48 to +53
diff[dependency] = compareUrl(dependency, previous, current);
}
}
for (let dependency in diff) {
diff[dependency].changes = await getDiff(diff[dependency].api);
}
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to do something like this to avoid the second loop?

Suggested change
diff[dependency] = compareUrl(dependency, previous, current);
}
}
for (let dependency in diff) {
diff[dependency].changes = await getDiff(diff[dependency].api);
}
const urls = compareUrl(dependency, previous, current);
diff[dependency] = {
...urls,
changes: await getDiff(urls.api)
}
}
}

Comment on lines +66 to +68
for (let dep in diff) {
console.log(asciiDependencyDiff(dep, diff[dep].api));
}
Copy link
Member

Choose a reason for hiding this comment

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

you could do

Suggested change
for (let dep in diff) {
console.log(asciiDependencyDiff(dep, diff[dep].api));
}
for (const {api} of diff) {
console.log(asciiDependencyDiff(dep, api));
}

Comment on lines +74 to +76
for (let dep in diff) {
console.log(`<${diff[dep].html}>`);
diff[dep].changes.forEach((change) => {
Copy link
Member

Choose a reason for hiding this comment

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

same here

Suggested change
for (let dep in diff) {
console.log(`<${diff[dep].html}>`);
diff[dep].changes.forEach((change) => {
for (const {html, changes} in diff) {
console.log(`<${html}>`);
changes.forEach((change) => {

Copy link
Member Author

Choose a reason for hiding this comment

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

excellent suggestions, thank you! I was thinking some more about this and am wondering if it might be better to keep the release notes generation out of sdk-release. e.g. it it's npm installable then releasers will be able to open a terminal and:

npx js-sdk-release-notes | pbcopy
# or
npx js-sdk-release-notes 5.0.432 | pbcopy
# or
npx js-sdk-release-notes 5.0.430...5.0.432 | pbcopy

Copy link
Member

Choose a reason for hiding this comment

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

that would be really cool!

@wsbrunson
Copy link
Member

one change i would consider is building up a string in your function and then doing console.log(result) instead of building it through console.log. may be easier said than done though so just a suggestion

@jshawl jshawl closed this Mar 6, 2024
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