This PR is a proposal to fix #1609 by enabling Augeas errors to be retrieved

[abeverley]

This PR is a proposal to fix #1609 by enabling Augeas errors to be retrieved

As per #1609 it is not possible to retrieve errors from Augeas if it is failing. This PR adds a new command to be able to retrieve them.

I have not added a test, as there are currently no tests at all for the Augeas command.

I thought about automatically retrieving and adding any error information to a failing Augeas command, but I decided that is over-engineering things too much and that a developer should decide how they want to handle errors and retrieve them manually if they wish to.

Please review and merge, or let me know how to improve it further.

[abeverley]

I notice that the test xt/author/critic-progressive.t seems to be failing. This passes on my local machine and I am not sure what is causing it to fail here - does it need fixing in order to progress this PR?

[ferki]

Thanks for this example PR to expand on your idea!

First thing I noticed is that the PR template is partially used as a git commit message, and partially missing (the checklist).

I'll take a look why the commitlint step did not catch this format as a failure, and we may have to rebase this afterwards, and fix the commit message.

It would also be nice to have the checklist back in the description to see how it shapes up over time, and to make sure we don't forget any item.

This PR adds a new command to be able to retrieve them.

I have a slightly better sense of the scope through this example. Let's find out if this is the best way to solve this on the interface. If we could have a more generic design, I would favor that over having to manually map each Augeas command to Rex code.

I have not added a test, as there are currently no tests at all for the Augeas command.

I would argue that lack of tests is a strong indicator that they should be added latest now before changing the code. There are many reasons to start with this, and prove that:

• the test suite could catch defects in the new functionality
• the proposed code change makes the new failing tests pass
• the behavior of the existing functionality is not affected

We may add the missing tests separately first (since they are fully lacking). At a minimum, I would like to make sure to have a single first commit adding initial tests only for this scope (and the second commit)

I thought about automatically retrieving and adding any error information to a failing Augeas command, but I decided that is over-engineering things too much and that a developer should decide how they want to handle errors and retrieve them manually if they wish to.

That sounds useful, especially given I almost exclusively operate any automation code in "strict mode" to bail out upon first error (set -e/set -o errexit in Bash, or `-feature => ['exec_autodie'] in Rex, etc.). We may add automatic retrieval later, or perhaps add it to the current basic error checking.

Please review and merge, or let me know how to improve it further.

In short we'd need to address at least the following:

• missing tests for the proposed change
• commmit message
• checklist (changelog entry)
• current test failures

I notice that the test xt/author/critic-progressive.t seems to be failing. This passes on my local machine and I am not sure what is causing it to fail here - does it need fixing in order to progress this PR?

Yes, it's there to prevent errors, and it needs fixing.

It might be passing for you locally because:

• the purpose of Test::Perl::Critic::Progressive is to ensure we don't add more perlcritic violations to the code base, only cleanups ("boy scout rule", aka. "leave the code base in no worse condition than it was found")
• on first run it accepts all pre-existing violations and passes
• it caches the list of perlcritic violations in the xt/author/.perlcritic-history file to use it as a baseline for future comparisons

Please make sure to:

• first run these tests on the current default branch
• delete the cached results if it was already run on some other branch, and run on the default branch again
• the new branch passes this test too (=don't add new violations)

The list of new violations doesn't seem too bad, though:

• one empty return
• two double quoted strings that doesn't need interpolation
• one new duplicate literal (errors; it perhaps already indicates that a generic interface passing commands verbatim to Augeas would be a good idea?)

[ferki]

What's the format returned? Is a scalar a good fit, or it would be better as an array (as indicated by the plural in errors)?

Also perhaps add a ; at the end.

[abeverley]

Thanks @ferki, just following up on this too. Sorry, I thought the template was more of a checklist, I didn't realise it was a template to retain in the same format. Thanks also for the information on the critic - that makes sense.

I'll close this for the time being as per #1609, and I can open a new one if/when required.