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.

Sign up for GitHub

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

Jump to bottom

Fix handling of fullwidth characters in AlignArray #2710

Merged
merged 1 commit into from
Jan 30, 2016
Merged

Fix handling of fullwidth characters in AlignArray #2710

merged 1 commit into from
Jan 30, 2016

Conversation

seikichi
Copy link
Contributor

I fixed the handling of fullwidth characters in Style/AlignArray.

The following code:

puts 'Ｒｕｂｙ', [1,
                 2]

yield the offence:

tmp/fullwidth.rb:5:19: C: Align the elements of an array literal if they span more than one line.
                  2]
                  ^

I chose the gem unicode-display_width to calculate display width because ...

$ rubocop -V                                                                                                                                                                                                                                    [~/src/rubocop]
0.36.0 (using Parser 2.3.0.1, running on ruby 2.1.8 x86_64-darwin15.0)

@seikichi seikichi force-pushed the align-array-with-fullwidth-chars branch from 5a1f7ff to 41eca5b Compare January 23, 2016 20:53
@jonas054
Copy link
Collaborator

Interesting. I've noticed that Emacs seems to indent/align the same way in the presence of these fullwidth characters. With some fonts the alignment is pretty good. With others it's a bit off, but I suppose that's just the way it is.

The change also affects some of the other cops that include AutocorrectAlignment, so perhaps you can add a few more spec examples.

@rrosenblum
Copy link
Contributor

Interesting, I didn't know this was an issue. The commit looks good to me. Slightly annoying that we need to add another dependency, but I am glad we do not need to handle that logic ourselves.

@seikichi
Copy link
Contributor Author

OK, I'll add more spec examples, thanks!

@seikichi seikichi force-pushed the align-array-with-fullwidth-chars branch from 41eca5b to b7dceda Compare January 25, 2016 21:30
@seikichi
Copy link
Contributor Author

Updated and rebased. I added tests against cops using AutocorrectAlignment.check_alignment.

@seikichi seikichi force-pushed the align-array-with-fullwidth-chars branch from b7dceda to f7c15a6 Compare January 25, 2016 22:47
@jonas054
Copy link
Collaborator

👍 Looks good to me. Note to self: see if display_column() can be used when fixing #2703.

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 30, 2016

You'll have to rebase on top of the current master.

@seikichi seikichi force-pushed the align-array-with-fullwidth-chars branch from f7c15a6 to 363a87e Compare January 30, 2016 10:05
@seikichi
Copy link
Contributor Author

Rebased.

bbatsov
bbatsov reviewed Jan 30, 2016
View reviewed changes
CHANGELOG.md
@@ -31,6 +31,7 @@
* [#2664](https://github.com/bbatsov/rubocop/issues/2664): `Performance/Casecmp` can auto-correct case comparison to variables and method calls without error. ([@rrosenblum][])
* [#2729](https://github.com/bbatsov/rubocop/issues/2729): Fix handling of hash literal as the first argument in `Style/RedundantParentheses`. ([@lumeet][])
* [#2703](https://github.com/bbatsov/rubocop/issues/2703): Handle byte order mark in `Style/IndentationWidth`, `Style/ElseAlignment`, `Lint/EndAlignment`, and `Lint/DefEndAlignment`. ([@jonas054][])
* [#2710](https://github.com/bbatsov/rubocop/pull/2710): Fix handling of fullwidth characters in `Style/AlignArray`. ([@seikichi][])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, you actually fix more than this in the current version of the commit, so I'd suggest extending this changelog entry and changing the commit message itself.

@seikichi
Fix handling of fullwidth characters in some cops
e383249 
I fixed the handling of fullwidth characters in following cops:

- `Style/AlignArray`
- `Style/AlignParameters`
- `Style/FirstParameterIndentation`
- `Style/IndentAssignment`
- `Style/IndentationConsistency`
@seikichi seikichi force-pushed the align-array-with-fullwidth-chars branch from 363a87e to e383249 Compare January 30, 2016 11:37
@seikichi
Copy link
Contributor Author

Thanks for your feedback. I changed the commit message and the changelog entry, and rebased.

bbatsov added a commit that referenced this pull request Jan 30, 2016
@bbatsov
Merge pull request #2710 from seikichi/align-array-with-fullwidth-chars
1946f26 
Fix handling of fullwidth characters in some cops
@bbatsov bbatsov merged commit 1946f26 into rubocop:master Jan 30, 2016
@bbatsov
Copy link
Collaborator

bbatsov commented Jan 30, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seikichi @jonas054 @rrosenblum @bbatsov