rubocop
Add guideline about empty strings inside interpolation.
I apologise for the pluralism of opinions. It doesn’t seem that we need a rule for a case like this. It is stylistic, quite unfrequent I believe, and some may say they prefer it explicit. Not to say that it has nothing to do with interpolation specifically, it’s just hard to detect other similar cases with static analysis, eg r = [“prefix”, cond ? “foo” : “”].join.
I searched on github for the two forms listed in the PR:
"#{condition ? 'foo' : ''}" had 1.2k results:
"#{'foo' if condition}" had 1.4k results:
Given the scale of rubocop, I do not know if this counts as frequent, but it is at least relevant information.
As to your point about the rule being stylistic, is that not the point of the ruby style guide? I'm open to discussion on whether this style is preferred, but I presumed it being a stylistic choice does not disqualify it from inclusion.
This is a confirmation that both styles take place. Do you see a particular reason to enforce one over the other?
The modifier if form is shorter, and only contains the relevant logic, while the ternary form has redundant logic for assigning an empty string.
Is being shorter a goal?
assigning an empty string
What do you mean by “assigning”?
In general, I’m not against the cop. But is there a place for such an edge case in the style guide?
A more generic rule could sound like:
Instead of a ternary that returns an empty string in one of the branches, in an expression where the result will be in some way concatenated with another string, prefer using an inline “if” instead
But I still think it’s not worth adding such a guideline.
Is being shorter a goal?
If code can be made more concise without sacrificing readability, performance or functionality, then yes, I think making it shorter is a goal.
What do you mean by “assigning”?
Assigning is probably the wrong word-- stating? creating?
But is there a place for such an edge case in the style guide?
I was told to open a PR, and it was my understanding that Style cops should have equivalent rules in the style guide. If that's not the case, I'm happy to close this PR and just focus on the cop itself.
without sacrificing readability
Don’t we sacrifice readability here by making it return a nil implicitly?
stating? creating?
Returning? But with the widespread frozen_string_literal there’s even no excessive object initialisation. It’s slightly less efficient, as this string won’t be the same object in a different file, like nil, but still. I don’t think an overhead like this is worth optimising for in a general case.
Style cops should have equivalent rules in the style guide
This style guide is not only about style, despite its name. And it has guidelines that readers users from introducing costly mistakes in their code.
I tend to agree that every cop should have a rationale behind it, but there are so many cops that putting this all into the style guide will make the style guide hard to read.
Off the top of my head, I can’t tell what the rule of adding vs not adding a guideline is. As far as I know, for rubocop-rails and rails-style-guide there is always a guideline that backs the cop. Not something I know of other extensions or the main style guide.
Don’t we sacrifice readability here by making it return a nil implicitly?
i see it like this: modifier form: "if condition then string" ternary form: "if condition then string otherwise no string"
the fact that nothing happens if the condition isn't true is, to me, self-evident in the modifier form, and so it is unnecessary to state it by using the ternary form.
I think its probably just personal opinion, though.
I don’t think an overhead like this is worth optimising for in a general case.
I wasn't thinking in terms of performance, just in terms of code clarity.
:shrug: I thought a PR for the style guide would be mandatory for a style cop. My bad. Some general advice for the github code search: append -is:fork. Results are pretty overstated otherwise, though it doesn't really change proportions that much in the end. It has some extra troubles like not considering ""-style strings in interpolation but that starts to get pretty complicated for a regex.
It's 40%/60% which is pretty close.
I would say that not everyone will agree with the shorter version. Ruby has some constructs that look a bit weird when compared to other languages and I would count using 'foo' if bar as a shorthand for nil in else as one of those. I find it similar to this:
a = nil
a = foo if bar
# or the longer form
a = nil
if false
a = foo
end
You don't need that first assignment but at least for me it looks pretty weird without it. I feel similar about this cop.
Reopening this because the cop enforcing this style was merged, and ended up including a link to this section of the style guide, so I figured it made sense for this to be in the style guide after all.
+1, it's quite confusing seeing this in my rubocop output, only to have the link be invalid:
Style/EmptyStringInsideInterpolation: Do not return empty strings in string interpolation. (https://rubystyle.guide#empty-strings-in-interpolation)
@bbatsov mind taking another look?
