[munin-run] cleanup - #1061
Conversation
cgzones
commented
Aug 27, 2018
cgzones
commented
- format with perltidy
- fix some perlciritc warnings
Pull Request Test Coverage Report for Build 2428
💛 - Coveralls |
|
There's a lot of whitespace changes here... |
e8608d1 to
b700f18
Compare
|
I split it up |
sumpfralle
left a comment
sumpfralle
left a comment
There was a problem hiding this comment.
Please see my two comments below.
| require Pod::Usage; | ||
| Pod::Usage::pod2usage( | ||
| -verbose => 99, | ||
| -verbose => 99, |
There was a problem hiding this comment.
Is this alignment of operators really a common style recommendation?
(Python's PEP8 defines exactly the opposite)
There was a problem hiding this comment.
It's the default perltidy setting
There was a problem hiding this comment.
I used to be very fond of it, as it makes the code a little more readable. But it has to be automated, which is okay with perltidy.
But what I prefer nowadays is the cleanliness of diffs, and usually this adds a lot of whitespace noise. It also makes git blame a headache to work with. Which is a tool that I use a lot ot understand the context of when the current code was written.
This is invaluable to be able to patch things efficiently according to the original intend. Moreover, code that I wrote 6 months ago feels just like code someone else wrote 😉 so...
There was a problem hiding this comment.
Instead of git blame you could use git log -p -M --follow --stat -- path/to/your/file
There was a problem hiding this comment.
Err... Not really.
I don't really want to spend brain cycles on something that could be easily automated for me ;)
That said, a git blame can be enhanced to cope with it. But let's do it once for all just prior to releasing.
There was a problem hiding this comment.
Anyway: thank you both for sharing your hints :)
What shall we do about the alignment of operators?
There was a problem hiding this comment.
What shall we do about the alignment of operators?
Let's defer all tidy changes to when we'll enforce the usage of perltidy.
| paranoia => $paranoia, | ||
| }); | ||
| $config->reinitialize( { | ||
| %{$config}, ## no critic qw(ValuesAndExpressions::ProhibitCommaSeparatedStatements) |
There was a problem hiding this comment.
Just wild guessing: is this expression supposed to merge the config hash together with the additional hash value and supply this new hash as an argument to the reinitialize method?
Maybe there is a better way to achieve this without overriding the policy?
There was a problem hiding this comment.
Yeah, that would be better, but I did not yet find a way, input welcome.
There was a problem hiding this comment.
Using a temporary var is always possible, and usually even preferred
There was a problem hiding this comment.
@cgzones: would you try to work around this (by using a separate variable?) one in order to save the warning override?
There was a problem hiding this comment.
I do not see a way to use a temporary variable :-(
There was a problem hiding this comment.
@steveschnepp: I think, it is time to prove your a temporary var is always possible claim :)
b700f18 to
a02819a
Compare
a02819a to
6dc1897
Compare
|
dropped perltidy changes |
|
ping |
|
@steveschnepp: what do you think about the remaining changes? |
|
@steveschnepp: the rest looks OK to me. Should we merge it? |
|
@steveschnepp: there are only a few lines of changes left in the discussion. Should we merge these? |
4 participants
