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

update rubocop and enable four new cops, re-enabling two that have been fixed #200

Merged
merged 1 commit into from Sep 17, 2020

Conversation

jmkoni
Copy link
Contributor

@jmkoni jmkoni commented Sep 16, 2020

…en fixed

* Update Rubocop from
  [0.90](https://github.com/rubocop-hq/rubocop/releases/tag/v0.90.0)
  to
  [0.91](https://github.com/rubocop-hq/rubocop/releases/tag/v0.91.0),
  enabling:
  * [`Lint/UselessTimes`](rubocop/rubocop#8702)
  * [`Layout/BeginEndAlignment`](rubocop/rubocop#8628)
  * [`Lint/ConstantDefinitionInBlock`](rubocop/rubocop#8707)
  * [`Lint/IdentityComparison`](rubocop/rubocop#8699)
  re-enabling after bug fixes:
  * [`Bundler/DuplicatedGem`](rubocop/rubocop#8666)
  * [`Naming/BinaryOperatorParameterName`](rubocop/rubocop#8664)
@jmkoni jmkoni requested a review from searls September 16, 2020 19:55
@searls
Copy link
Contributor

searls commented Sep 16, 2020

This all seems totally legit!

Is there a matchin rubocop-performance release to bump?

@jmkoni
Copy link
Contributor Author

jmkoni commented Sep 16, 2020

@searls not yet!

@searls
Copy link
Contributor

searls commented Sep 16, 2020

Awesome, I'm 👍 to merge & cut a release

@jmkoni jmkoni merged commit 4b503e4 into master Sep 17, 2020
@jmkoni jmkoni deleted the update-to-0.91 branch September 17, 2020 14:06
@will
Copy link
Contributor

will commented Sep 22, 2020

Hi, Lint/ConstantDefinitionInBlock breaks for sorbet's enums which look like this:

# (1) New enumerations are defined by creating a subclass of T::Enum
class Suit < T::Enum
  # (2) Enum values are declared within an `enums do` block
  enums do
    Spades = new
    Hearts = new
    Clubs = new
    Diamonds = new
  end
end

@searls
Copy link
Contributor

searls commented Sep 22, 2020

@will lol, Sorbet isn't making this project any easier, Will

@will
Copy link
Contributor

will commented Sep 22, 2020

Yeah…

I figured it'd be an interesting thing to try on my new project, and like it's… okay. But not great. It's for sure caught some things I wouldn't have otherwise and the language server thing and vim+ale is neat. Though there are defiantly costs, and when I go over and do a little bit of Crystal I'm like "oh here is a type system that actually works with duck typing and isn't just annoying all the time" and then I get sad at sorbet.

@searls
Copy link
Contributor

searls commented Sep 22, 2020

@will does it make sense for us to either (1) go out of our way to ensure standard doesn't screw with sorbet? I haven't been keeping up with the Ruby 3 type protocol stuff, or (2) invest in an additional flag or mode that allows standard to dynamically switch to a different rubocop config for Sorbet projects (the same way we do for Ruby 1.8-2.3 projects)?

@will
Copy link
Contributor

will commented Sep 22, 2020

Ruby 3 type protocol stuff,

I'm probably not all the way up to date, so this might be wrong, but my understanding is that ruby 3 will have out-of-line type info files that you can write called "rbs". Sorbet has a combination of in-line type signatures and also optionally out-of-line "rbi" files. Right now "rbs" and "rbi" are incompatible, but I think sorbet will at least optionally be change to support the "rbs" files.

as for the options, I think if anything (2). I have no idea how popular sorbet actually is, and so far the standard vs. sorbet problems have been due to very sorbet specific things rather than things that I think would be problems with ruby 3 typing.

But I'd also be okay with option (3), make Will maintain a few ignores in .standard.yml. Let me know if you want me to stop reporting sorbet things, it's cool :)

@jmkoni
Copy link
Contributor Author

jmkoni commented Sep 22, 2020

I'm open to you continuing to report with the chance that we might not fix it 😬 but if you open up an issue and it gets engagement, that at least can let us know how many other standard users are running into the same problem.

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.

None yet

3 participants