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

Better clarity on case-when indentation #741

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NewAlexandria
Copy link
Contributor

This replaces PR #422, and maintains a minimum of overall new lines.


I just realized that #422 was still open, and re-reading it I think that this edit is more in-line with the intent of the PR, and all the feedback. I hope it will an easy merge.

This replaces PR rubocop#422, and maintains a minimum of overall new lines
@bbatsov
Copy link
Collaborator

bbatsov commented May 3, 2019

I'm fine with the spirit of the change, I just wonder if this should be a separate rule or not. It's kind of orthogonal to the indentation of when itself.

README.md Outdated
@@ -341,11 +345,26 @@ Translations of the guide are available in the following languages:
puts 'Too long!'
when Time.now.hour > 21
puts "It's too late"
when :minus_op, :minus_minus_op
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given this example, I think we're talking more about Good and Better than about Bad and Good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's just one other precedent in the style guide. I'm happy to make a simple Better block, and try to overall keep the examples 'deduped' to limit newlines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated @bbatsov. A smaller diff now, too.

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.

2 participants