-
Notifications
You must be signed in to change notification settings - Fork 8.1k
contribution guidelines: change to say updating changelog is required #5586
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
Changes from all commits
d180036
35118d0
ceaba9c
597f4a4
a0c7584
89c9a85
1477af8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,14 @@ | ||
| <!-- | ||
| ## PR Checklist | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this right place? I believe the check list is for maintainers only. I'd prefer the template here:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Problem investigation and results should be in the issue. This checklist is all items that by our defined process is supposed to be done by the contributor except for the one new item this PR specifically adds and the one @markekraus asked me to add. Change Summary is supposed to be what you call the "fix description" and we the link in the checklist describes how to produce this. Change Summary is more appropriate because not all PRs are fixes.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree, but in fact, we see another. The initial description in a Issue is almost always different from what we need to do in PR. Moreover, in most cases Issues do not contain a final opinion. +1 for Change Summary. Non-blocking comment. |
||
|
|
||
| If you are a PowerShell Team member, please make sure you choose the Reviewer(s) and Assignee for your PR. | ||
| If you are not from the PowerShell Team, you can leave the fields blank and the Maintainers will choose them for you. If you are familiar with the team, feel free to mention some Reviewers yourself. | ||
| Note: Please mark anything not applicable to this PR `NA`. | ||
| - [ ] [PR has a meaningful title](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission) | ||
| - [ ] Use the present tense and imperative mood when describing your changes | ||
| - [ ] [Summarized changes](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission) | ||
| - [ ] [Update the changelog](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---changelog) | ||
| - [ ] User facing [Documentation needed](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission) | ||
| - [ ] Issue filed - Issue link: | ||
| - [ ] [Change is not breaking](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#making-breaking-changes) | ||
| - [ ] [Make sure you've added a new test, if existing tests do not effectively test the code changed](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#before-submitting) | ||
| - [ ] [Add `[feature]` if the change is significant or affectes feature tests](https://github.com/PowerShell/PowerShell/blob/master/docs/testing-guidelines/testing-guidelines.md#requesting-additional-tests-for-a-pr) | ||
|
|
||
| For more information about the roles of Reviewer and Assignee, refer to [CONTRIBUTING.md](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md). | ||
|
|
||
| --> | ||
| ## PR Summary | ||
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.
Based on previous experience I don't like the requirement.
Finally, we can easily automate it. I previously sent @daxian-dbw a sample script. If we'll to keep the PR headers neat, we'll get a neat log of the automatic way.
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.
We reject changes which contain both formatting and functional changes to the same file. This is not the same thing at all.
Unless we say, maintainers are not part of the community, I don't think we can say this. The PowerShell DSC community and other communities I've participated in have this requirement without issue.
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.
https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md#fixing-an-issue Item number 7.
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.
Finally, I'm not sure it's easy to keep the PR
headersclean and therefore automate it. By header, I assume you mean the message, and the message is often too limited to use for as the changelog. If you mean the notes, since they are unstructured, they suffer other problems.If the author refuses to do this, the maintainer can always update the changelog for them, but in the DSC community, this was rarely an issue.
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.
DSC repository has less than 100 PR in the last two years. We have so much in two months. We will actually have a conflict in every PR with this file.
Previously, the team required to update the file and then forget it. Why? Probably because it's not a functional change that everyone forgets and ignores.
I'm sorry, I don't understand the usefulness of this file while there is an open GitHub - any consumer can at any time not only look at the history of changes, but also the history of the code.
This is a secondary element to me. If this element requires so much effort, I immediately think that it should be removed or automated. If we can't use a PR header/description, we could think about using a commit header/description.
Uh oh!
There was an error while loading. Please reload this page.
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 is an attribute on the file to use a Union merge, which is supposed to prevent merge conflicts. This may cause some issues in the file, but should produce a usable file. Many other projects use this method.
When you were looking at the DSC Repository, did you count all the submodules? I asked the DSC team and DSC had 43 PRs in November across all the repos.
As to the purpose of the file, most people that just want to consume the project would like a simplified way of viewing what has been changed in the project from their perspective. This is not what the PR or the commit log is about and this needs to be reviewed in context to make sure it's being expressed clearly. For example, #2253 suggest we include a link to the changelog in the What's New section of the documentation.
Even if it were automated, translating from the contributor focused messaged of the PR to the PowerShell user-focused message in the changelog is what is taking the time. Automating gathering the PR descriptions and links would only solve at best half the problem.
Uh oh!
There was an error while loading. Please reload this page.
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.
No, I follow your link. I wonder do they have one changelog for all repos? If no they never have merge conflicts as we had previously with the file.
I'm not the first person who wants this "kill" feature:
https://pypi.python.org/pypi/gitchangelog#changelog
https://www.npmjs.com/package/auto-changelog
https://github.com/skywinder/github-changelog-generator
Also I found this project very interesting (and links there too).
My comments isn't blocking.
Uh oh!
There was an error while loading. Please reload this page.
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 this one is the most interesting: https://github.com/skywinder/github-changelog-generator But, it needs a LOT of work to be usable. This issue needs to be fixed, and we would like to be able to group by tags.
This issue needs to be fixed:
github-changelog-generator/github-changelog-generator#493
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.
Tracking Issue #5614