[doc] Rex::Commands::File: correct optional argument syntax

[rwp0]

Clarify commands docs taking optional arguments such as append_if_no_such_file

I hope I did all the changes except creating an issue first which I promise to do the next time :-)

#1526

Checklist

• based on top of latest source code
• changelog entry included
• tests pass in CI
• git history is clean
• git commit messages are well-written

[rwp0]

Pull down action image 'wagoid/commitlint-github-action:4.1.12'
  /usr/bin/docker pull wagoid/commitlint-github-action:4.1.1[2](https://github.com/RexOps/Rex/runs/6863140237?check_suite_focus=true#step:2:2)
  Error response from daemon: Head "https://registry-1.docker.io/v2/wagoid/commitlint-github-action/manifests/4.1.12": received unexpected HTTP status: 500 Internal Server Error
  Warning: Docker pull failed with exit code 1, back off 6.593 seconds before retry.
  /usr/bin/docker pull wagoid/commitlint-github-action:4.1.12
  Error response from daemon: Head "https://registry-1.docker.io/v2/wagoid/commitlint-github-action/manifests/4.1.12": received unexpected HTTP status: 500 Internal Server Error
  Warning: Docker pull failed with exit code 1, back off 9.5[3](https://github.com/RexOps/Rex/runs/6863140237?check_suite_focus=true#step:2:3)1 seconds before retry.
  /usr/bin/docker pull wagoid/commitlint-github-action:4.1.12
  Error response from daemon: Head "https://registry-1.docker.io/v2/wagoid/commitlint-github-action/manifests/4.1.12": received unexpected HTTP status: [5](https://github.com/RexOps/Rex/runs/6863140237?check_suite_focus=true#step:2:5)00 Internal Server Error
Error: Docker pull failed with exit code 1

commitlint failing with ISE 500

[ferki]

Hmm, the two jobs failed for external reasons, so now I've re-run them. Only the commit check fails now, and I'll come back for a review as soon as that passes too.

Let me know if you need help with that earlier, though. Feel free to join the chat on Matrix or IRC too, it might be easier to coordinate there when we're both online.

[rwp0]

Could you shrink my commit message from 78 to 50?

I'll also join Matrix after registration.

Thanks.

[ferki]

Could you shrink my commit message from 78 to 50?

Sure, how about going with Clarify optional arguments of file commands? My reasons:

• short and direct, and clearly stating of the scope of changes inside the commit
• directly reusable as a changelog entry
• reading that indicates clear expectations from the reviewer: go through the entire Rex::Commands::File module, and look for any optional arguments that might be not yet indicated clearly

---

Meanwhile, a new version of Perl::Tidy has been released so I updated the test suite and the codebase formatting on the default branch already.

Please also rebase your changes on top of the default branch in order to test them against the current state.

After this PR passes all tests, I'll do a full review.

[rwp0]

I'm not very git/GitHub-savvy though will try my best to rebase and amend if possible considering your last comment 🙂

[rwp0]

Whatever, giving up on this PR 😞

I git with Intellij IDEA and rebase/amend/squash breaks everything everytime for me.

Feels too dangerous for simple edits.

This are just too complex for simple things, easy things should be easy right?

I also feel like enforcing commitlint is also not the best for "layman" contributors like myself (maybe it'd be good for developers for internal standartization) 🙂

Or commits could be edited by maintainers manually, isn't ticking Allow edits by maintainers is supposed for that?

Anyways this just prevents simple contribution (second PR failing).

[ferki]

TL; DR

Don't feel discouraged if it didn't succeed at first try. The tests are there to help avoid common mistakes, and it's fairly standard to have multiple versions of the same idea before something is merged.

I see at least these options to go forward:

• Please follow the standard procedure and open an issue that explains the original problem you experienced. It's something along the lines of optional parameters of file commands are not documented well, and you already have good examples. Then any people interested may pick that up and fix that in their own rhythm.
• We keep focus on improving the documentation in this PR, keep iterating on this change together until it's good enough to merge, and keep discussing different aspects on different threads (perhaps on even better suited channels).

They are not excluding each other, and none of them are mandatory.

---

Long version

Whatever, giving up on this PR

I'm sad to hear that. I don't think it's lost, though.

I git with Intellij IDEA and rebase/amend/squash breaks everything everytime for me.

Feels too dangerous for simple edits.

This are just too complex for simple things, easy things should be easy right?

"Simple" and "easy" does not mean the same for everybody or for all tools. Take your time to figure it out, and ask for help if stuck.

I don't have experience with IntelliJ IDEA. Standard git itself provides plenty of direct options to polish the branch history including commit messages before merging. I linked rebase pointers before, but GitHub also has documentation about Changing a commit message and Rebasing commits against a branch.

I'm usually available on Matrix for more interactive help, and also offer open source office hours free of charge if you prefer to work in pair in a planned manner.

I also feel like enforcing commitlint is also not the best for "layman" contributors like myself (maybe it'd be good for developers for internal standartization)

Thanks for the feedback on that.

The Contributing guide explains the established expectations and practices around contributions. This includes Git workflow and how to write a git commit message (which article also explains why good commit messages matter).

Or commits could be edited by maintainers manually, isn't ticking Allow edits by maintainers is supposed for that?

That's an option, yes.

I don't change other's commits before merging to preserve the work of the original author intact. Technically, however, I can try to rebase this branch for you (GitHub allows editing fork branches on PRs), and I can also rebase it on my own before merging. We may be able to aim for one of these options here.

Anyways this just prevents simple contribution

The current codebase is a result of 100+ volunteers' collective work over more than a decade. To avoid chaos and keep it sustainable, there are some standards in place based on what we learned over time. These expectations are explained in the Contributing guide and tested automatically to provide fast feedback even if a reviewer or maintainer is not available immediately.

All contributions are measured against the same standards to ensure and maintain consistency. The "rules" are part of the repository, so these are also up to discussions (separately from this PR) and can be subject to contributions.

[...] (second PR failing).

Oh, I don't consider the previous PR a failure at all! ❤️ It enabled us to identify important points about how to improve documentation content and consistency, and also to expand the scope of automated feedback.

From my perspective, that was a PR without an issue where we could have discussed details before work is invested. While working on that we discovered a broader scope than we originally expected. Instead of iterating on the same PR until it is good enough to merge, there was a decision to open a new PR instead.

[rwp0]

@ferki thank you for mentoring me through PR 1530 (this one) on Element to successfuly amend and rebase my commit to pass checks and be a candidate for reviews.

I'm glad I could learn git first class citizen concepts through you and this PR in particular.

[ferki]

Thank you for coming back to this PR, and I'm glad you could update it! 🎉 ❤️

It took me a while to go through all cases in Rex::Commands::File, but in the end I find the proposed changes correct in the scope of the "mark optional parameters optional" idea, and consistent with the current state of the rest of the file.

I left a note about one of the changes mostly for future reference for future maintainers who might read this PR. No action needed about that.

---

I found two more cases to add on top of the existing ones, though:

• the %options of file() is also optional, so let's mark that similarly too
• the @params of template() is also optional: it takes either a %params (named parameters) or \%params (named parameters in a hash reference)

I'm not sure how it's best to mark the second one. I'm leaning towards choosing the preferred/recommended notation, which is %params to reduce punctuation around what is being passed. I'm open to better alternatives if there is an even better proposal.

---

Please update the PR by including those two as well, and choose the way you are comfortable with:

1. The easiest way is probably to amend the commit like last time, and push again.
2. If you want, you can push it as a new commit, and we can squash the two commits into one before merging. This might be useful for practice, but sounds more work overall :)
3. Alternatively, if you don't want to update the PR, that's fine too. I can add the above as a second commit on my own before merging, just let me know that I should not wait for the update.

[rwp0]

Thank you for coming back to this PR, and I'm glad you could update it! tada heart

It took me a while to go through all cases in Rex::Commands::File, but in the end I find the proposed changes correct in the scope of the "mark optional parameters optional" idea, and consistent with the current state of the rest of the file.

I left a note about one of the changes mostly for future reference for future maintainers who might read this PR. No action needed about that.

I found two more cases to add on top of the existing ones, though:

• the %options of file() is also optional, so let's mark that similarly too
• the @params of template() is also optional: it takes either a %params (named parameters) or \%params (named parameters in a hash reference)

I'm not sure how it's best to mark the second one. I'm leaning towards choosing the preferred/recommended notation, which is %params to reduce punctuation around what is being passed. I'm open to better alternatives if there is an even better proposal.

Please update the PR by including those two as well, and choose the way you are comfortable with:

1. The easiest way is probably to amend the commit like last time, and push again.
2. If you want, you can push it as a new commit, and we can squash the two commits into one before merging. This might be useful for practice, but sounds more work overall :)
3. Alternatively, if you don't want to update the PR, that's fine too. I can add the above as a second commit on my own before merging, just let me know that I should not wait for the update.

yeah, I also thought that file and template can be optional, but couldn't imagine any to be useful without parameters.

If you want, you can push it as a new commit, and we can squash the two commits into one before merging. This might be useful for practice, but sounds more work overall :)

I chose this option for practice, please tell me when and how we can do it

[ferki]

yeah, I also thought that file and template can be optional, but couldn't imagine any to be useful without parameters.

The parameter itself is still optional whether there's a point to use it like that or not :) Some examples for them without using optional parameters, though:

• template: the template content itself might define how to get its variable parts; for example it can retrieve the hostname inside the template on its own, instead of having to retrive it outside the template just to pass it as a parameter
• file: file '/path/to/file'; without anything else is a shortcut to ensure the file exists (think like touch).

If you want, you can push it as a new commit, and we can squash the two commits into one before merging. This might be useful for practice, but sounds more work overall :)

I chose this option for practice, please tell me when and how we can do it

Good news is that "squashing" is also done with the already used git rebase command, just passing --interactive. GitHub has a short tutorial about Using Git rebase on the command line about this.

What I would specifically do here is the following:

• run git rebase --interactive HEAD~2 to start editing the last 2 commits
• in the action list replace pick with fixup for the second commit (so this commit becomes part of the first commit without keeping its own commit message)
• check the result of the interactive rebase in the local git history: there should be a single commit with Clarify optional arguments of file commands as its subject, no message body, and with all changes contained in its diff
• update this PR with git push --force-with-lease

[rwp0]

changes addressed