-
Notifications
You must be signed in to change notification settings - Fork 721
Enhance "Making your PR Merge Worthy" section #3028
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
base: devel
Are you sure you want to change the base?
Conversation
- Fix the number-verb agreement in a bullet point - Add a link to the RST spec and re-word the embedding
I find the title "Creating changelog fragments" too similar to "Creating a changelog fragment".
Thanks for your Ansible docs contribution! We talk about Ansible documentation on Matrix at #docs:ansible.im if you ever want to join us and chat about the docs! We meet on Matrix every Tuesday. See the Ansible calendar for meeting details. We welcome additions to our weekly agenda items too. You can add the |
Testing PRs | ||
----------- | ||
|
||
Adding tests for your PR makes it a stronger candidate for being merged. |
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'm not sure if tests are required per any official policy, or if there is any
nuance for things that shouldn't include tests.
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 cases where tests aren't necessary are obvious to contributors (like sending in a typo fix). We probably shouldn't give them any idea along the lines of "by hey, you can actually ignore this requirement". They'll ask if unsure.
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.
Hmm okay, I've rephrased to be more ambiguous but I think a call out to "Hey tests are good" should be included for first time contributors especially.
Capitalizing these helps to distinguish between the yaml/rst format and file extension.
3a2fcd4 only capitalizes some acronyms I have no idea why this is failing. |
@pkingstonxyz Likely one of those transitive blips that just happen from time to time. The check was fine when I re-ran it so whatever the cause, it was unrelated to your commit. |
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
This PR seeks to increase the clarity and scope of the "Making your PR Merge Worthy"
section of the The Ansible Development Cycle page.
It updates the wording of the changelog section to fix some small grammar issues and
includes a link to the RST specification.
This PR also adds a section "Testing PRs" with links to information on Integration and
Unit tests because one of the most common comments on PRs is some variation of "Can you write some tests for this?"