Fix Lint/AmbiguousOperator warnings #1021 #1022

Open
wants to merge 1 commit into
from

Projects

None yet

5 participants

@hotovson
Contributor
hotovson commented Sep 16, 2016 edited

fix part of #1021

@mattwynne
Member

So, this might we one we need to discuss @cucumber/cucumber-ruby

@tooky and I, when pairing, have developed a convention that we use parentheses when we care about the return value, and not when we don't.

I'm struggling to find the reference that inspired that style, but this article is a reference.

I am fond of this style, but I'm more interested in consistency than pushing one particular style. If this parentheses-everywhere rules is easier to check with rubocop, I can live with it.

What are other people's thoughts?

@nodo
Contributor
nodo commented Sep 22, 2016

@mattwynne That is an interesting convention and I wouldn't mind to keep ๐Ÿ‘ .

If we decided to keep the convention, it would be great to ignore these violations in the .rubocop.yml file and add a paragraph in CONTRIBUTING.md that explains the code conventions of the project.

@threedaymonk
Contributor

@mattwynne That's the convention I've long used myself, although, like you, I can't remember where I originally drew the inspiration. I suspect it flows naturally from Ruby's precedence rules:

do_something transform(munge(a, b))

is easier to parse mentally than the equivalent but paren-less

do_something transform munge a, b
@tooky
Member
tooky commented Sep 22, 2016

I think it's nice to use ruby's loose syntax for conventions - I remember Jim Weirich was a proponent of using {} blocks when you care about the return value and do; end when you don't.

But if we are going to have conventions like that, I like @nodo's suggestion to document them.

There are others we developed, like always returning self from commands. We should definitely write them down.

@hotovson
Contributor

rebased to current master

@nodo
Contributor
nodo commented Sep 23, 2016 edited

As previously suggested by @brasmusson in this comment I changed the PR message to "fix part of #1021" to inhibit Github to automatically close the related issue.

@hotovson
Contributor

@nodo can you re-run Travis CI tests? it seems false negative

@mattwynne
Member

I agree we should at least document this. I don't suppose there's a way to actually get the linter to check for that convention is there?

@nodo
Contributor
nodo commented Sep 25, 2016

@hotovson thanks for your work, I have restarted the CI tests and indeed it was a false negative.

@mattwynne unfortunately I don't think there is a easy way to get rubocop check some custom conventions. I will have a deeper look.

@hotovson
Contributor

@nodo @mattwynne IMHO the only way is by custom cops

@mattwynne
Member

So what's the consensus with this one @cucumber/cucumber-ruby? I hate to waste @hotovson's good work, but it does seem as though a documented convention would work better in this instance, rather than a hard style check. Do we agree?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment