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

Style/StringConcatenation: flexibility settings #8470

Closed
zverok opened this issue Aug 6, 2020 · 5 comments · Fixed by #8634
Closed

Style/StringConcatenation: flexibility settings #8470

zverok opened this issue Aug 6, 2020 · 5 comments · Fixed by #8634

Comments

@zverok
Copy link
Contributor

zverok commented Aug 6, 2020

New Style/StringConcatenation is obviously useful, but it seems it needs more flexibility to not harm code readability.

For example,
"http://" + domain"http://#{domain}"
is a good suggestion, but
"http://" + fetch_domain(some, params, with: COMPLEX[calculation]) + "/endpoint"
is more vague.

Maybe it would make sense to limit the suggestion by either concatenated expression character count, or by its complexity (number of sub-expressions or something)?..

@marcandre
Copy link
Contributor

Are you suggesting to not raise offense for your second example, or to raise but not auto-correct that?

Because there's an argument for

# bad
"http://" + fetch_domain(some, params, with: COMPLEX[calculation]) + "/endpoint"
# good
domain = fetch_domain(some, params, with: COMPLEX[calculation])
"http://#{domain}/endpoint"

We can't autocorrect this way, of course, but I feel an offense is still justified.
Maybe a different cop could look for "string#{with_very_long_expressions(:inside, :a, :string)}" and suggest to extract and name the expression...

@zverok
Copy link
Contributor Author

zverok commented Aug 20, 2020

Are you suggesting to not raise offense for your second example, or to raise but not auto-correct that?

Probably "provide (some) method of limiting the offenses only to atomic (?) clauses, or to clauses of some Max complexity/charcter length".
I somewhat overcomplicated the example, but even something simpler, like "http://" + domain(environment: 'production') + "/endpoint" might seem less readable with interpolation (and not deserving one more instance var.).

Probably, "do not autocorrect when there is complex expressions, but still raise" might be a sutiable compromise.

@dvandersluis
Copy link
Member

@marcandre I'd love the 2nd example to raise but not autocorrect, because I've run into some weird autocorrections, such as

if Rails.env.production?
  "/#{id}"
else
  "dev/#{id}"
end + '/video'

becoming

"#{if Rails.env.production?
     "/#{id}"
   else
     "dev/#{id}"
end}/video"

Which is wrong for a number of reasons. I'd rather just fix it by extracting a variable, as you suggested.

@marcandre
Copy link
Contributor

marcandre commented Sep 2, 2020

I'm 👍 in keeping offenses but adding some criteria so auto-correction does not happen.

I'm thinking these should imply no correction:

  • nested interpolation
  • multi-line

(PR for this welcome)

I'm personally less enthusiastic about adding config options in general / judgement about length / other complexity of code, or not raising offenses, but the right examples may be persuade me or other people

@dvandersluis
Copy link
Member

@marcandre sure! I'll try to put together a PR.

dvandersluis added a commit to dvandersluis/rubocop that referenced this issue Sep 3, 2020
…n if any of the parts are too complex.

Complex includes multiline statements, heredocs, nested interpolations and expressions with blocks.
@mergify mergify bot closed this as completed in #8634 Sep 3, 2020
mergify bot pushed a commit that referenced this issue Sep 3, 2020
…y of the parts are too complex.

Complex includes multiline statements, heredocs, nested interpolations and expressions with blocks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants