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

Add auto-correct to BlockAlignment cop #1078

Merged
merged 2 commits into from
Aug 14, 2014

Conversation

barunio
Copy link
Contributor

@barunio barunio commented May 10, 2014

This is a modified version of #1002. The original PR went beyond the scope of this one cop, so this no longer includes the extra functionality. (That will be taken care of by a new cop, which I'll open a PR for in a bit.)

Note that the class disables the ClassLength cop, because it is too long with the extra functionality. It didn't make sense to me to extract the autocorrect into a module, since it isn't shared anywhere, just to satisfy the length limit. Let me know if there's some other approach that you think makes more sense here.

@bbatsov
Copy link
Collaborator

bbatsov commented May 11, 2014

Looks OK to me, but let's here what @jonas054 thinks as well. You should also add a changelog entry for this.

@jonas054
Copy link
Collaborator

@barunio The AutocorrectAlignment module has most of the funtionality you need. All but the "end statement is not on its own line" part. I wonder if you could find a way to use AutocorrectAlignment, adding the missing ingredient to that module. It could perhaps solve the class length problem.

@jonas054
Copy link
Collaborator

Or actually a new cop for "end statement is not on its own line" similar to the one in #1079 makes much more sense. Then you can use AutocorrectAlignment without changes.

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 12, 2014

@barunio Ping

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 17, 2014

@jonas054 Would you look into wrapping this PR and #1079? Seems that @barunio is MIA and I don't like keeping PRs opened for months.

@jonas054
Copy link
Collaborator

Sure.

@jonas054 jonas054 self-assigned this Jul 17, 2014
@barunio
Copy link
Contributor Author

barunio commented Jul 17, 2014

@bbatsov @jonas054 Ah, apologies! I've just been swamped, so I lost track of this entirely. As this PR was my responsibility, I'm happy to re-examine it today and adjust for the above comments. I think I have another PR open that also needs to be closed out.

@jonas054
Copy link
Collaborator

Thanks, @barunio. I'll hold my horses.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 8, 2014

It'd be good if this were wrapped soon, as I plan to release the next stable version in the beginning of next week.

@barunio
Copy link
Contributor Author

barunio commented Aug 11, 2014

ok, will try to wrap it up today

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 9d052f4beb6df86a1fe01943500281e9c74cec3c on barunio:block_alignment_autocorrect into 106da8b on bbatsov:master.

@barunio
Copy link
Contributor Author

barunio commented Aug 14, 2014

@jonas054 @bbatsov

I split up the "end statement not on its own line" into a separate cop as requested. But, I don't see any easy way to use the AutocorrectAlignment module for the other changes, since that module is designed to align all lines in the node, whereas I just want to align the last line with the first. If one of you wants to edit further to change that, feel free. I won't have time to make further changes.

I'm not sure if BlockEndNewline is the best name for the new cop; let me know if you prefer something different.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling fcf8548 on barunio:block_alignment_autocorrect into 106da8b on bbatsov:master.

bbatsov added a commit that referenced this pull request Aug 14, 2014
@bbatsov bbatsov merged commit c166dde into rubocop:master Aug 14, 2014
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 14, 2014

Guess this will do for now. Thanks!

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

Successfully merging this pull request may close these issues.

4 participants