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 PercentLiteralDelimiters auto-correct #1251

Conversation

hannestyden
Copy link
Contributor

Make PercentLiteralDelimiters preserve the indentation of the contained expression.

expression +
closing_newline + closing_delimiter + reg_opt
expression_indentation + expression + closing_newline +
closing_indentation + closing_delimiter + reg_opt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels like a bit too much effort for just replacing two characters in a string. Does anyone have a suggestion on how to improve it? The "problem" is that it is not known exactly which characters should be replaced (they could be any delimiter pair) nor the position of the last character (it could be followed by regular expression modifiers).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, seem pretty complex (even before your changes). Off the top of my head I can't think of anything better, but perhaps @jonas054 and @yujinakayama have some alternative ideas.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't. The code looks quite clear to me, even if it is a bit complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bbatsov @jonas054 OK. Dis- and re-assembling a node is probably the best general approach for auto-correction.

Thanks for reviewing.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 13, 2014

LGTM.

@jonas054
Copy link
Collaborator

There's a typo in the commit title: PercentLiterDelimiters

@hannestyden hannestyden changed the title Fix PercentLiterDelimiters auto-correct Fix PercentLiteralDelimiters auto-correct Aug 13, 2014
@hannestyden
Copy link
Contributor Author

@jonas054 Typo fixed. Thanks for noticing.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 13, 2014

@hannestyden Just noticed there's no changelog entry.

Make it preserve the indentation of the contained expression.
@hannestyden
Copy link
Contributor Author

@bbatsov Changelog entry added.

bbatsov added a commit that referenced this pull request Aug 13, 2014
…auto-correct-fix

Fix PercentLiteralDelimiters auto-correct
@bbatsov bbatsov merged commit 104d4bb into rubocop:master Aug 13, 2014
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 13, 2014

👍

@hannestyden
Copy link
Contributor Author

@bbatsov Thanks! 🙇

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

Successfully merging this pull request may close these issues.

3 participants