Fix RuboCop offenses

[utkarsh2102]

These offenses appear when you create a gem with
bundle gem foo and run rubocop over it.

Initially, there were around 45 offenses detected,
but with #3731 and this, the number of offenses
have been reduced to 0.

@bronzdoc, as promised, here's this 🎉

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>

---

Tasks:

• Describe the problem / feature
• Write code to solve the problem
• Write tests
• Get code review from coworkers / friends

I will abide by the code of conduct.

[simi]

I'm sorry I have missed https://github.com/rubygems/rubygems/pull/3731/files, but I think we should ship no rule overrides, just follow the default ones (to not bring any another opinion in there actually) and create on CI new test to create gem and run rubocop on top of that to ensure no offences are present on skeleton.

[deivid-rodriguez]

@simi I tend to recall that we discussed this in the past and we want to encourage using double quotes by overriding this default in the generated gem skeleton. That's my opinion anyways :)

[deivid-rodriguez]

I'm happy with adding a CI test to ensure that rubocop is green on generated gems.

[utkarsh2102]

I'm sorry I have missed https://github.com/rubygems/rubygems/pull/3731/files, but I think we should ship no rule overrides, just follow the default ones (to not bring any another opinion in there actually) and create on CI new test to create gem and run rubocop on top of that to ensure no offences are present on skeleton.

AFAICU, the team prefers the use of double quotes so in case there's a case of string interpolation, there'll be no inconsistency with both sorts of "StringLiterals". And I tend to agree with this.

[deivid-rodriguez]

@utkarsh2102 Yeah, I believe that's why the bundler team uses double_quotes internally. The question is whether we want to promote that also on new gems. My opinion is yes to that as well :)

[utkarsh2102]

The question is whether we want to promote that also on new gems. My opinion is yes to that as well :)

I wholeheartedly agree with this as well 😄

[deivid-rodriguez]

@simi Well, my recollection was wrong. I double checked rubygems/bundler#6455, and the only opinions there are:

• @colby-swandale's: follow rubocop defaults (EDIT: changed "community" to "rubocop" because it's debatable that single_quotes is the "community default").
• Mine: ship a .rubocop.yml with the overrides we want to ship.

So, it seems that I'm losing this battle too 😅

[deivid-rodriguez]

@indirect @hsbt Any opinions here?

[simi]

AFAICU, the team prefers the use of double quotes so in case there's a case of string interpolation, there'll be no inconsistency with both sorts of "StringLiterals". And I tend to agree with this.

I do not fight for any rule and actually bundler/rubygems standards are not related here since it is new gem skeleton, not actual part of runtime.

I'm just trying to say that if we would like to integrate rubocop in new gem skeleton, we should not do any decision on configuration. The same we do for rspec and minitest, just minimal default integration needed to be able to start testing without bringing any opinions.

[deivid-rodriguez]

Yeah, that's the debate here. My opinion is that it is fine that we bring some opinions, specially with double_quotes, since a relevant part of the community actively changes this default. And it also provides a sample configuration, which is nice to get started in my opinion.

[utkarsh2102]

I do not fight for any rule and actually bundler/rubygems standards are not related here since it is new gem skeleton, not actual part of runtime.
I'm just trying to say that if we would like to integrate rubocop in new gem skeleton, we should not do any decision on configuration. The same we do for rspec and minitest, just minimal default integration needed to be able to start testing without bringing any opinions.

Of course. It's just opinion-based. #3731 was raised because I didn't like rubocop throwing around 45 offense to a very new (skeleton) gem.
I personally would prefer double_quotes (and understood that team would prefer that too because of inconsistencies caused by the string interpolation thingy). So raised that and now this.

I am perfectly fine with whatever the outcome of this discussion is, but I think it's important that we, at least, discuss this.

And obviously, all in good faith! ❤️❤️

[deivid-rodriguez]

Yeah, as long as we ship an offense free rubocop setup, I'm fine with any outcome here.

Let's wait for some more opinions, and if the current balance (2 to 1) doesn't change, let's remove the generated .rubocop.yml file and switch the templates to whatever rubocop likes.

[simi]

I didn't like rubocop throwing around 45 offense to a very new (skeleton) gem.

Yes, I think we all have the same goal. Provide skeleton without any rubocop violations. The question is how to get to this state :) My opinion was written already, anyway I'm ok with any solution.

🤔 Next step should be actually pull request to integrate rubocop check in bundled CI scripts as well and have CI green by default. 🥳

[deivid-rodriguez]

@utkarsh2102 Do you want to give that test a try? In the new gem specs, add a spec that runs rubocop inside the generated gem, and checks that there are no errors.

[utkarsh2102]

@utkarsh2102 Do you want to give that test a try? In the new gem specs, add a spec that runs rubocop inside the generated gem, and checks that there are no errors.

Sure, I'd love to do that! 😄

[hsbt]

I don't like rubocop's default rule. In fact, many of projects disable it rule.

• https://github.com/rails/rails/blob/master/.rubocop.yml#L9
• https://github.com/github/rubocop-github/blob/master/config/default_deprecated.yml#L4
• https://github.com/discourse/rubocop-discourse/blob/master/default.yml#L10

In my opinion, we put DisabledByDefault: true to default rubocop configuration on new gem. But I'm okay to this pull request without FrozenStringLiteralComment style. Because the default of frozen string literal is canceled at Ruby 3.0. There is no reason to enforce it.

[deivid-rodriguez]

Yeah, have a look at rubocop/rubocop#5306 for a discussion on what the preferred quote style is in the ruby community. It's about 50/50, although the most popular projects usually use double quotes.

I don't mind about FrozenStringLiteralComment. I think it doesn't hurt even if it won't be the default, but I don't mind if we turn it off.

However, I believe DisabledByDefault: true limits the usefulness of adding rubocop too much. I use DisabledByDefault: true all the time for big projects with very inconsistent style, so that I can gradually enforce consistency, but for new projects I think using all the power of rubocop is better.

[utkarsh2102]

But I'm okay to this pull request without FrozenStringLiteralComment style. Because the default of frozen string literal is canceled at Ruby 3.0. There is no reason to enforce it.

I don't mind, but how about, we consider this road:
-> Ruby 3.0 is released with the default frozen string literal canceled.
-> FrozenStringLiteralComment cop in RuboCop if fixed to not enforce this behavior.
-> And finally then we remove this here.

This might be sounding crazy, but the whole idea of doing this is to not show any offenses by running rubocop in the first place.
If we don't enfore FrozenStringLiteralComment now, the skeleton gem will have around 10 offenses.

[deivid-rodriguez]

@utkarsh2102 What @hsbt is suggesting is to add

Style/FrozenStringLiteralComment:
  Enabled: false

to the generated gem rubocop configuration and not add any frozen_string_literal: true comment to the templates. That approach would also have no offenses.

[utkarsh2102]

@utkarsh2102 What @hsbt is suggesting is to add

Style/FrozenStringLiteralComment:
  Enabled: false

to the generated gem rubocop configuration and not add any frozen_string_literal: true comment to the templates. That approach would also have no offenses.

Ah, sure. This means that we are definitely shipping a .rubocop.yml? 😄

[deivid-rodriguez]

Haha, good question 😆.

It seems that we have a tie (2-2), although I'm still giving this some more thought and I might even reconsider my vote 😅

Let's hold on a bit on that, but you can definitely add the spec already since everybody wants that.

[deivid-rodriguez]

@indirect @hsbt Any opinions here?

Also forgot to ask @bronzdoc!

[indirect]

I am strongly in favor of having tests for this, good suggestion @utkarsh2102!

Personally, I would like my new gems to use double-quotes. 😅 I am happy with removing frozen string literals, to get us ahead of the Ruby 3 curve, and I would agree with @deivid-rodriguez to leave the rules on by default since this is a new project.

That said, I am in favor of this work even if the consensus ends up not agreeing with my personal preferences. 😄

[utkarsh2102]

Finallyyy! The CI is greeeen! 😭🎉

[deivid-rodriguez]

I can't believe this is green finally 😭

Great job 💪

[utkarsh2102]

I can't believe this is green finally 😭
Great job 💪

Thanks to you for helping and being patient with me ❤️

[deivid-rodriguez]

Since this PR fulfills our main goal of having a clean rubocop run on new gems by default, I'll merge this unless anybody has objections.

As @simi suggested, we can discuss further tweaks on the default configuration later.

[indirect]

Ah, I just noticed the change from %q to double quotes in the gemspec—this isn't blocking, but the reason I did that is so that gem descriptions can contain both double and single quotes (like %q{Helps users' "friends" meet each other} can just be typed directly into the string. It's super frustrating to break a string by typing an english sentence that describes the gem. :)

[utkarsh2102]

Hi @indirect,

Ah, I just noticed the change from %q to double quotes in the gemspec—this isn't blocking, but the reason I did that is so that gem descriptions can contain both double and single quotes (like %q{Helps users' "friends" meet each other} can just be typed directly into the string. It's super frustrating to break a string by typing an english sentence that describes the gem. :)

I hear you, I really do!
And I agree that it's frustrating indeed to have things broken by a mere sentence. And I'll be even happy to revert that back to using %q, but in order to do so and at the same time make sure we've 0 offenses shown by RuboCop, we need to put this into .rubocop.yml and this will be yet another "opinionated" config that we ship --- which was the whole discussion we had above :P

Personally, I am happy to do so but again, we will need to discuss more about whether we want to ship a default .rubocop.yml or not.

The best way forward, I think, is

• We get this merged so we at least have an offenseless skeleton gem, which will be very good!
• Simultaneously, I can start a thread about this on Slack and we can discuss more about this and decide how the things will going to be by the next release.

That said, I can even revert back to %q and disable Style/PercentLiteralDelimiters and Style/RedundantPercentQ by default, but we will go in the same loop again.. :P

So let me know what you think? 😅

[indirect]

Oh I think we should merge first! I called that out as not blocking on purpose. 😄 Thanks for trying to work with us.

[utkarsh2102]

Thanks for trying to work with us.

It's both, an honor and a pleasure to do so ❤️

[deivid-rodriguez]

Awesome, merging this pending further discussions about magic string literals comments, or specific exclusions to the default string quotes rule in the gemspec 👍.

Thanks so much for your patience @utkarsh2102!