On 2014-15-07 24:08, Kylo Ginsberg wrote:
So now putting on my opinionated hat ;)
On Mon, Jul 14, 2014 at 12:04 PM, Andy Parker <a...@puppetlabs.com
<mailto:a...@puppetlabs.com>> wrote:
On Mon, Jul 14, 2014 at 10:56 AM, Kylo Ginsberg <k...@puppetlabs.com
<mailto:k...@puppetlabs.com>> wrote:
For the build and test pipeline integration, I think it would be
great to have it as part of the PR process. However, since every
once in a while we have things enter that don't go through a PR
(normally quick little fixes and such), then we should also have
this running in Jenkins.
Yep, +1 on it being in Jenkins also.
And we have merges...
I don't think that it should be executed as part of "rake spec",
since that would slow it down for us whenever we run them, and it is
really one of those things that once we get into the habit of
abiding by the rules, we very rarely actually break them. What I've
done before is have a separate job in Jenkins for running static
checks. It gets kicked off after the spec tests passed and in
parallel with the next stage after specs (in our case, that is
packaging).
In Rahul's PR it's a separate rake task. It's faster than than a full
Jenkins spec matrix, and platform-independent, so we could possibly run
it in parallel with specs.
and then there's been some discussion on the PR around these two
cops:
Style/AndOr
Lint/AssignmentInCondition
Each of those two checks catch coding patterns which both are a
source of some bugs and, at the same time are idiomatic in
certain cases. So there's room for discussion on those two.
The only idiomatic part of the and/or one that we'd get rid of is
failure statements: do_something() or raise "Noooo!!!". I'm willing
to drop that for the slightly more wordy:
if !do_something()
raise "Nooooo!!!!"
end
In fact, I often like that construction better because it is more
future proof. If it turns out that you need to add extra actions in
the error case, then there isn't any need to perform refactor first.
I agree. I'm +1 with jettisoning and/or entirely.
+1, not needed.
I'm also +1 on adding Lint/AssignmentInCondition, both because I've seen
the occasional bug that would be caught by this, and more often I've
seen some confusion for code readers that would be caught by this ("did
they really mean an assignment here?"). I believe it can be addressed
simply by parenthesizing the expression.
I am fine with parenthesizing, will not catch every such bug anyway
since it is common to have parentheses and && || etc. It is no
replacement for review. May cause mork work fixing what we currently
have than what it gives in return. Don't mind though, and I will always
parenthesize my conditional assignments from now on (anyway).
That said, I recognize that assignment-in-conditions is widely used and
thus this one may be controversial, so I'm curious to hear others' thoughts.
And then there are a *bunch* more cops for a variety of
style/lint checks which we could consider enabling in addition
to the above. There's some documentation of the various cops in
the rubocop yaml files at:
https://github.com/bbatsov/rubocop/tree/master/config
I think we should avoid any purely formatting cops. Instead we
should figure out a common formatting utility to use and just take
code formatting out the path of a developer's whims.
Why have a separate tool? I'd rather have fewer tools if it gets the job
done, but I may be missing your point. That said, I'd be inclined to go
light on pure formatting cops unless they catch bugs or really help with
readability. Are the following uncontroversial?
Whitespace cleanup:
Style/TrailingBlankLine
ok
Style/TrailingWhitespace
ok
Style/IndentationConsistency
If this means no mixed spaces and tabs
Style/IndentationWidth
This can mean several things, such as how much something should be
indented - in general it should follow the rules, but there are
exceptions esp. when the same pattern is repeated and formatting makes
it harder to see that pattern (more mental capacity is required) - and
then people start fighting with the formatter and adding *no format*
here there and everywhere and soon we have to forbid *no format*.
Those four are primarily of interest to me b/c lots of editors do these
things automagically (good thing) so some PRs end up with unrelated
edits to fix the above. Might as well nuke them once and for all and get
cleaner PRs.
Similarly, alignment checkers catch readability issues:
Lint/BlockAlignment
Lint/DefEndALignment
Lint/EndAlignment
Okay, there are more I find tempting but that's enough for now ....
I think the basic hygiene checks (trailing spaces, no line break last,
etc.) can be enforced but that formatting is something best done by the
authoring tool / the author. Sure have a style checker / linter - but as
an aid, not a prescription.
- henrik
--
Visit my Blog "Puppet on the Edge"
http://puppet-on-the-edge.blogspot.se/
--
You received this message because you are subscribed to the Google Groups "Puppet
Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to puppet-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/puppet-dev/lq1ohn%24aej%241%40ger.gmane.org.
For more options, visit https://groups.google.com/d/optout.
