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 #3570] avoid if-modifiers on multi-line statements #3636

Merged
merged 1 commit into from Oct 24, 2016

Conversation

Projects
None yet
5 participants
@tessi
Contributor

tessi commented Oct 17, 2016

This implements a Cop to handle multiline if modifiers.

This is my first commit to this project, so I wouldn't mind some extra care before merging ;)
Especially the autocorrect I've implemented might have some edges I have not foreseen.

@scottmatthewman

Great cop! I've suggested some minor naming and textual changes.

Show outdated Hide outdated config/enabled.yml Outdated
Show outdated Hide outdated lib/rubocop/cop/style/multiline_if_modifier.rb Outdated
Show outdated Hide outdated lib/rubocop/cop/style/multiline_if_modifier.rb Outdated
Show outdated Hide outdated lib/rubocop/cop/style/multiline_if_modifier.rb Outdated
Show outdated Hide outdated lib/rubocop/cop/style/multiline_if_modifier.rb Outdated
@tessi

This comment has been minimized.

Show comment
Hide comment
@tessi

tessi Oct 17, 2016

Contributor

Ha! Of course my cop introduced some failures on the rubocop codebase itself. Ideal debugging cases for me. I'm investigating.

Contributor

tessi commented Oct 17, 2016

Ha! Of course my cop introduced some failures on the rubocop codebase itself. Ideal debugging cases for me. I'm investigating.

@tessi

This comment has been minimized.

Show comment
Hide comment
@tessi

tessi Oct 17, 2016

Contributor

Thanks @scottmatthewman for the review! I fixed my typos based on your comments.

In addition to that, the cop got some improvements in detecting offenses (multi-line conditions are OK, whereas multiline if-bodies are not) and auto-correction (better handling of code indentation).

Some places in the rubocop codebase were targeted by the new cop. Those are fixed as well.

Contributor

tessi commented Oct 17, 2016

Thanks @scottmatthewman for the review! I fixed my typos based on your comments.

In addition to that, the cop got some improvements in detecting offenses (multi-line conditions are OK, whereas multiline if-bodies are not) and auto-correction (better handling of code indentation).

Some places in the rubocop codebase were targeted by the new cop. Those are fixed as well.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 18, 2016

Collaborator

I just wonder if this should be a separate cop or part of the existing cop about modifier expressions. @jonas054 @alexdowad Any thoughts?

Collaborator

bbatsov commented Oct 18, 2016

I just wonder if this should be a separate cop or part of the existing cop about modifier expressions. @jonas054 @alexdowad Any thoughts?

@alexdowad

This comment has been minimized.

Show comment
Hide comment
@alexdowad

alexdowad Oct 18, 2016

Contributor

@bbatsov I don't see any other cop which looks like it overlaps with the intent of this one. IfUnlessModifier is quite different; perhaps it should be given a more descriptive name.

@tessi has some code in there for adjusting the indentation of autocorrected code... I feel like we should have a generic helper method for that somewhere already. Or if we don't, we should.

@tessi Nice work! One thing you still need to do is to squash these commits.

Contributor

alexdowad commented Oct 18, 2016

@bbatsov I don't see any other cop which looks like it overlaps with the intent of this one. IfUnlessModifier is quite different; perhaps it should be given a more descriptive name.

@tessi has some code in there for adjusting the indentation of autocorrected code... I feel like we should have a generic helper method for that somewhere already. Or if we don't, we should.

@tessi Nice work! One thing you still need to do is to squash these commits.

@Drenmi

This comment has been minimized.

Show comment
Hide comment
@Drenmi

Drenmi Oct 18, 2016

Collaborator

@tessi has some code in there for adjusting the indentation of autocorrected code... I feel like we should have a generic helper method for that somewhere already. Or if we don't, we should.

I am inclined to say that this should be done by another cop (leveraging the inspection loop), or this concern will start to tangle itself into a lot of cops whose primary concerns should be something else. WDYT?

Collaborator

Drenmi commented Oct 18, 2016

@tessi has some code in there for adjusting the indentation of autocorrected code... I feel like we should have a generic helper method for that somewhere already. Or if we don't, we should.

I am inclined to say that this should be done by another cop (leveraging the inspection loop), or this concern will start to tangle itself into a lot of cops whose primary concerns should be something else. WDYT?

@tessi

This comment has been minimized.

Show comment
Hide comment
@tessi

tessi Oct 18, 2016

Contributor

Thanks for your feedback. I'll rebase and squash my commits in a minute.

Regarding the indentation code, I had similar thoughts: First, I assumed some other Cop will take care of indentation -- which is actually the case. Unfortunately, this produced some additional whitespace warnings (when using the auto-correct functionality) -- but those whitespace error were not the users fault (but the fault of my Cop) -- and I felt bad for the user. On top of that, the produced indentation was a little off (the end was not indented properly). The latter could probably be fixed in some whitespace-Cop, the former issue might be harder to fix.
Anyhow, I thought that a cop's autocorrect should produce 'good' code from the start, which is why I decided to implement proper indentation in my Cop.

I assumed there was a "indent my random piece of code automatically"-helper, but found none. So, I re-used some functions of the AutocorrectAlignment module, which was the closest thing I could find (apparently, implementing indentation has its own pitfalls -- for example that the user can configure the indentation width).

IMO cops should produce 'good' code when auto-correcting (including proper indentation), which is why we should have some helpers to indent code. The question is if those should be written in this PR (I'm willing to try) or not. I'd say it should be done in a later PR and be reviewed separately.

Contributor

tessi commented Oct 18, 2016

Thanks for your feedback. I'll rebase and squash my commits in a minute.

Regarding the indentation code, I had similar thoughts: First, I assumed some other Cop will take care of indentation -- which is actually the case. Unfortunately, this produced some additional whitespace warnings (when using the auto-correct functionality) -- but those whitespace error were not the users fault (but the fault of my Cop) -- and I felt bad for the user. On top of that, the produced indentation was a little off (the end was not indented properly). The latter could probably be fixed in some whitespace-Cop, the former issue might be harder to fix.
Anyhow, I thought that a cop's autocorrect should produce 'good' code from the start, which is why I decided to implement proper indentation in my Cop.

I assumed there was a "indent my random piece of code automatically"-helper, but found none. So, I re-used some functions of the AutocorrectAlignment module, which was the closest thing I could find (apparently, implementing indentation has its own pitfalls -- for example that the user can configure the indentation width).

IMO cops should produce 'good' code when auto-correcting (including proper indentation), which is why we should have some helpers to indent code. The question is if those should be written in this PR (I'm willing to try) or not. I'd say it should be done in a later PR and be reviewed separately.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 20, 2016

Collaborator

IMO cops should produce 'good' code when auto-correcting (including proper indentation), which is why we should have some helpers to indent code. The question is if those should be written in this PR (I'm willing to try) or not. I'd say it should be done in a later PR and be reviewed separately.

Ideally, yes, but I wouldn't say that's a major issue. Keep in mind that sometimes the proper generated code would be dependent on RuboCop's config.

Collaborator

bbatsov commented Oct 20, 2016

IMO cops should produce 'good' code when auto-correcting (including proper indentation), which is why we should have some helpers to indent code. The question is if those should be written in this PR (I'm willing to try) or not. I'd say it should be done in a later PR and be reviewed separately.

Ideally, yes, but I wouldn't say that's a major issue. Keep in mind that sometimes the proper generated code would be dependent on RuboCop's config.

@tessi

This comment has been minimized.

Show comment
Hide comment
@tessi

tessi Oct 24, 2016

Contributor

Ideally, yes, but I wouldn't say that's a major issue. Keep in mind that sometimes the proper generated code would be dependent on RuboCop's config.

That's right. It's probably unfeasible to follow (and maintain) all configs in every cop.
Just to be clear: do you suggest to remove the indentation parts of this PR? If so, I'd remove it and rebase.
Are there any other things I should do before merging?

Contributor

tessi commented Oct 24, 2016

Ideally, yes, but I wouldn't say that's a major issue. Keep in mind that sometimes the proper generated code would be dependent on RuboCop's config.

That's right. It's probably unfeasible to follow (and maintain) all configs in every cop.
Just to be clear: do you suggest to remove the indentation parts of this PR? If so, I'd remove it and rebase.
Are there any other things I should do before merging?

@Drenmi

This comment has been minimized.

Show comment
Hide comment
@Drenmi

Drenmi Oct 24, 2016

Collaborator

IMO cops should produce 'good' code when auto-correcting (including proper indentation), which is why we should have some helpers to indent code.

I foresee the helpers would almost immediately become a shortcut to "run all the indentation cops on this code." 😀

Collaborator

Drenmi commented Oct 24, 2016

IMO cops should produce 'good' code when auto-correcting (including proper indentation), which is why we should have some helpers to indent code.

I foresee the helpers would almost immediately become a shortcut to "run all the indentation cops on this code." 😀

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 24, 2016

Collaborator

Just to be clear: do you suggest to remove the indentation parts of this PR? If so, I'd remove it and rebase.

I guess that's fine.

Collaborator

bbatsov commented Oct 24, 2016

Just to be clear: do you suggest to remove the indentation parts of this PR? If so, I'd remove it and rebase.

I guess that's fine.

@bbatsov bbatsov merged commit 5b64541 into rubocop-hq:master Oct 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

bbatsov added a commit that referenced this pull request Oct 24, 2016

Merge pull request #3636 from tessi/feature/3570-guard-clauses-on-mul…
…ti-line-statements

[Fix #3570] avoid if-modifiers on multi-line statements

@tessi tessi deleted the tessi:feature/3570-guard-clauses-on-multi-line-statements branch Oct 24, 2016

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