-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add github-changelog script #11155
Add github-changelog script #11155
Conversation
Note that this script only catches milestoned pull requests. We're pretty consistently applying milestones as part of the merging process, but there might still be cases where that's missed. So it's important to check there are no merged PRs without a milestone left. Instead of listing milestoned PRs, an ideal solution would be to query PRs based on commits since the latest release tag. That would be more accurate, but it's harder to implement with the GitHub API, so I chose to go with milestoned PRs for reduced complexity. The difference schould be pretty much insignificant in practice. |
What happened with #10985? |
Hm, that apparently got entirely forgotten. 🤷 Somehow I couldn't find it in my local branches and I had the script still laying around, so I figured I hadn't published it yet. |
One comment: I'd sort the PRs on the creation date, and not the merged date, since the latter one is less meaningful. |
I think the merge date has more meaning because it relates to the chronological order in the git history. |
response = HTTP::Client.post("https://api.github.com/graphql", | ||
body: {query: query, variables: variables}.to_json, | ||
headers: HTTP::Headers{ | ||
"Authorization" => "bearer #{api_token}", |
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.
Convention is to use the title case here:
"Authorization" => "bearer #{api_token}", | |
"Authorization" => "Bearer #{api_token}", |
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 don't think this matters.
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.
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 don't think this matters.
Then you should be OK with the standard and not the deviation from it.
It LGTM, but since @bcardiff had a similar script, I think it makes sense to have him review this. Probably it's missing a few features from the one by Brian, but those can be adjusted after using 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.
Looks good. I used to run the changelog helper on the shards repo also.
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
This helper queries merged pull requests for a given milestone from the GitHub API and creates formatted changelog entries.
Resolves crystal-lang/distribution-scripts#114
Example output