Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SpaceBeforeFirstArg cop for multiline arguments #945

Merged
merged 1 commit into from
Apr 4, 2014

Conversation

cschramm
Copy link
Contributor

@cschramm cschramm commented Apr 3, 2014

I have this code:

SimpleCov.formatter = SimpleCov::Formatter::MultiFormatter[
    SimpleCov::Formatter::HTMLFormatter,
    SimpleCov::Formatter::RcovFormatter
]

The cop treats everything after the equal sign as arg1_with_space and this does in fact start with a space, but the regular expression searches for any line not starting with a whitespace, not just the first one.

@bbatsov bbatsov added the bug label Apr 3, 2014
@jonas054
Copy link
Collaborator

jonas054 commented Apr 3, 2014

Yep, you found a bug. My bad.

Code looks good, @cschramm! Please add a spec example that fails before your fix, and an entry in CHANGELOG.md as well.

@jonas054
Copy link
Collaborator

jonas054 commented Apr 3, 2014

Actually, I might have been too quick when endorsing the change. This cop, which is a lint cop, should not warn for setter calls without leading space (a.b=c). That's a different concern that's already handled by SpaceAroundOperators.

So changing the regexp is good. We should keep that. But we also need examples saying that setter calls without space are accepted by this cop, and that a first argument that is preceded by a space and contains a non-space right after a line break within it, is accepted.

We usually exclude setter calls by return if method_name.to_s.end_with?('=').

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 3, 2014

@jonas054 Won't it be simpler if we checked whether the method name ends with ! or ?. Are there any other cases that need handling?

@jonas054
Copy link
Collaborator

jonas054 commented Apr 3, 2014

There's one more that is not in any big need of handling, but I though could tag along, and that's

method"some string"

I can't think of any others. Well, there's

puts`ls`

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 3, 2014

@jonas054 I think those are all the cases that need handling, indeed. @cschramm, will you make the required changes or should we do them ourselves?

@cschramm
Copy link
Contributor Author

cschramm commented Apr 4, 2014

I can do so. Do you want to blacklist !, ?, " and ` or just whitelist =?

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 4, 2014

Whilelist assignment.

@cschramm
Copy link
Contributor Author

cschramm commented Apr 4, 2014

Done.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 4, 2014

@cschramm you'll have to squash and rebase on top of the current master. Looks good to me, otherwise.

@jonas054 Are you happy with the current state of the code?

@@ -26,6 +26,16 @@
inspect_source(cop, ['something[:x]'])
expect(cop.offenses).to be_empty
end

it 'accepts a method call with space before a multiline arg' do
inspect_source(cop, "something [\n 'foo',\n 'bar'\n]")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make this a bit more readable like this:

inspect_source(cop, ['something [',
                                  '  foo',
                                  '  bar',
                                  ']'])

@cschramm
Copy link
Contributor Author

cschramm commented Apr 4, 2014

Squashed and rebased.

@jonas054
Copy link
Collaborator

jonas054 commented Apr 4, 2014

👍 I'm happy with the changes.

bbatsov added a commit that referenced this pull request Apr 4, 2014
Fix SpaceBeforeFirstArg cop for multiline arguments
@bbatsov bbatsov merged commit 2ba0c08 into rubocop:master Apr 4, 2014
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 4, 2014

Thanks!

@cschramm
Copy link
Contributor Author

cschramm commented Apr 6, 2014

You're welcome 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants