Edits to grammar, markdown, and format to match current styleguide. #50
Conversation
|
Where should we document that reorder is a different commit at the end? Make it easy to see what changed in the first commit. I had this conversation already with the acls doc fixups |
|
So, @ferventcoder , we had a team discussion about this issue, and we don't think it's a good practice for us to separate commits in that way. The biggest reason for that is that these PRs also serve as a technical review, which means the document really needs to be reviewed as a whole, rather than just what's been changed. We've been bitten before when only the changes have gotten reviewed, so we'd like to avoid that in the future. |
|
@jbondpdx what part of the technical review as a whole cannot be reviewed? I guess not including me in your discussion you missed that you can do BOTH if you split the commits. |
|
Let me explain. When you split the commits up, I can go to the first commit and look at exactly what has changed without the second commit where things were reordered. OR I can click over to Files changed tab and see the entire set of changes as a whole and comment on them as well. It's a win win for everyone. What @psoloway is doing in that other PR wasn't exactly as I had intended when I asked for the changes to be split. I had asked that the FIRST commit be the changes to the doc, and the second commit be the reorder. With both changes up in the PR, you can get BOTH things you are looking for. |
|
Ah, I see what you're saying. And yes, I think your request was somewhat misunderstood. That said, I'm still not sure why they need to be separate commits if you're reading the entire document for correctness. We need devs to review what hasn't been changed as well as what has, so I'm still not seeing why it is important for the changes to be separated in this way. That said, we also don't want to do this because it's rather onerous. Reordering and editing aren't separate tasks; they're interdependent. So separating these wouldn't be a win for us at all, but would instead create an undue burden. |
It's hard to see what HAS changed when the order is different. If I can go to a specific commit and look at what has changed, and then go to the changes part of the PR and see it all together I can see the changes as a whole. The important part in some cases is seeing that the meaning of the changes hasn't been lost, which is not easy to see when the order is changed and the words are changed in the _same_ commit. See https://github.com/puppetlabs/puppetlabs-acl/pull/53/files#diff-04c6e90faac2675aa89e2176d2eec7d8R17 for an example of seeing what has changed. Now see https://github.com/puppetlabs/puppetlabs-acl/pull/53/files#diff-04c6e90faac2675aa89e2176d2eec7d8R53 for an OMG I have no idea if this is reordered or changed or both. If there are two distinct commits, I can go back to the first commit and look at what specifically changed.
I can totally understand what you mean here because it creates an undue burden on the reviewer as it's nearly impossible to see what has changed. Maybe there is a happy medium here? |
|
That all said, I love the help making the documentation better. I definitely think there is a happy medium we can achieve that keeps the burden reasonable for both sides. |
|
I put those links in using a split view, if you are using unified you may want to scroll up to see what the diff is comparing. |
|
Perhaps the first commit is the reorder and then subsequent commits are related to getting the wording right? That seems to be a more plausible method of going about it. Plus there are methods to compare the wording changes based on this methodology (which will avoid interactive rebases). |
|
Are we good to merge these changes? |
|
@jbondpdx @jtappa The commit message needs fixed to indicate DOC-1497. I did my best on a review of the actual changes. Please understand that my suggestions above were to help alleviate more work on your part as well in telling me what you changed. Without following known git best practices, I may need more hand-holding with regard to the actual changes. |
| @@ -17,95 +18,110 @@ This module adds a new exec provider capable of executing PowerShell commands. | |||
|
|
|||
| ##Module Description | |||
|
|
|||
| Puppet provides a built-in `exec` type that is capable of executing commands. This module adds a `powershell` provider to the `exec` type, which enables all of the `exec` parameters, such as `creates`, `onlyif`, `unless`, etc. This module is particularly helpful if you need to run PowerShell commands but don't know the details about how PowerShell is executed, since you can technically run PowerShell commands in Puppet without the module. | |||
| Puppet provides a built-in `exec` type that is capable of executing commands. This module adds a `powershell` provider to the `exec` type, which enables `exec` parameters, listed below. This module is particularly helpful if you need to run PowerShell commands but don't know the details about how PowerShell is executed, since you can technically run PowerShell commands in Puppet without the module. | |||
There was a problem hiding this comment.
Should we link to the parameters section in the listed below?
There was a problem hiding this comment.
There's already a link in the ToC to the reference, so a link is not necessary here. We're trying to avoid excessive linking in the readmes. The "listed below" was added in conjunction with the removal of the link to exec's documentation to help clarify we're specifically listing relevant parameters in the readme.
|
Updated. |
|
|
||
| #####`path` | ||
| Specifies the search path used for command execution. Valid options: String of the path, an array, or a semicolon-separated list. Default: Undefined. | ||
|
|
|
I'm +1 to merge. @joshcooper ? |
Edits to grammar, markdown, and format to match current styleguide.
|
👍 |
Per DOC-1497