In our controllers we use jBuilder to render json views for the API controllers. It appears that those templates aren't getting picked up by the cop.

app/controllers/api/v1/users_controller.rb:85:12: C: Template could not be found
    render "show"
           ^^^^^^^^^

I'll see what I can do about getting a PR up. Just wanted to get this documented and see if there were any thoughts on it.

There are two reasons why this is an unreliable solution:

• it doesn't know about things that have yet to be autoloaded
• it's non-deterministic with regards to Garbage Collection of classes. If you use Class.descendants in Test, where there is a pattern to dynamically define classes, GC is unpredictable for when those classes are cleaned up and removed by the GC.

cc @jhawthorn

The RailsControllerRender... cops seem confusing. I'm not sure what they want me to do. Can I get some examples for good and bad practices when using these cops?

Style/FlipFlop has the wrong namespace - should be Lint

Summary

Add rubocop-rails as Gem dependency when upgrading to the next 0.72 release of rubocop.

Details

Rails cops will be removed from RuboCop 0.72, therefore we need to use the rubocop-rails gem instead.

👋 ! In the Ruby working group, we're trying to improve the support for Ruby at GitHub. Part of that story is sharpening our development tools, and RuboCop seems like a great place to do some sharpening. Here's what I'd like to do:

• Audit the rules in this gem and github/github:
  • Do we want to preserve all these rules? (And should we "upstream" some from github/github?)
  • Are there rules we want to add?
• Assess the autocorrectability and feedback of the rules we decide to keep.
  • RuboCop should be a development aid, not a blocker
  • When it does get in a developer's way, it's important to empower the developer with clear, actionable, and informative feedback
  • Then, it's great if RuboCop could get itself out of the way, with autocorrect support whenever possible. Maybe that means backfilling autocorrect support for rules we keep.

I think there are a few other ways we could leverage RuboCop better, but refreshing this project a bit would be a solid start!

What do you think, are there other ways we can improve this project or get more value from it?

If you want to start on the goals described above, feel free to open an issue or PR with a proposed change.

It looks like there are several cops which implement various approaches to recommending:

render "path/to/template", { ... options ... }

Why are these cops separate? Could they be unified?

For example, one cop replaces render action: :edit => render action: "edit", but another replaces render action: "edit" => render "edit". So, could we jump right from render action: :edit" => render "edit"?

It seems rubocop has updated it's cops and removed some. I'm getting the following error.

/Users/user/.gem/ruby/2.4.2/gems/rubocop-github-0.5.0/config/default.yml: Style/AsciiIdentifiers has the wrong namespace - should be Naming
/Users/user/.gem/ruby/2.4.2/gems/rubocop-github-0.5.0/config/default.yml: Style/ClassAndModuleCamelCase has the wrong namespace - should be Naming
/Users/user/.gem/ruby/2.4.2/gems/rubocop-github-0.5.0/config/default.yml: Style/FileName has the wrong namespace - should be Naming
/Users/user/.gem/ruby/2.4.2/gems/rubocop-github-0.5.0/config/default.yml: Style/MethodName has the wrong namespace - should be Naming
Error: The `Lint/InvalidCharacterLiteral` cop has been removed since it was never being actually triggered.
(obsolete configuration found in /Users/user/.gem/ruby/2.4.2/gems/rubocop-github-0.5.0/config/default.yml, please update it)

Rubocop has removed certain cops that are still included in rubocop-github.

http://www.rubydoc.info/gems/rubocop/RuboCop/Config
https://github.com/github/rubocop-github/blob/master/config/default.yml#L67

I was just digging through the styleguide, and spotted that at least in github/github where we fail the build if frozen_string_literal: true is not set, the suggested code won't actually run.

Should we update the guidance here? Or be more clear that large is the keyword here, and guide as to how to correctly opt out of frozen_string_literal (I don't actually know how to)

Avoid using String#+ when you need to construct large data chunks. Instead, use String#<<. Concatenation mutates the string instance in-place and is always faster than String#+, which creates a bunch of new string objects.

# good and also fast
html = ""
html << "<h1>Page title</h1>"

paragraphs.each do |paragraph|
  html << "<p>#{paragraph}</p>"
end

I don't agree with the preference expressed in the guide, but I think I understand the issue. Consider this Ruby code:

class A 
  class << self 
    attr_accessor :some_attribute 
  end 
end 

class B < A 
end 

As far as I know, that's the only way to support usage like this:

A.some_attribute = :value_1 
B.some_attribute # => nil 
B.some_attribute = :value_2 
A.some_attribute # => :value_1 

I mean, using a class variable would cause it to be shared by A and B, which we don't want.

Does that sound right to you? Is there another way to implement that snippet above?

I think the same thing is true for alias, it only works for class methods inside class << self.

So, I don't agree with the preference, but I think that explains the difference, does that make sense?

Originally posted by @rmosolgo in #7 (comment)

Right now rubocop is pinned at 0.89.0 which is outdated with 1.3.1 being the latest.

This gem is being used by Super-Linter and I would like to be able to move it to the 1.0.0 branch to use the newer cops.

Thanks for putting this together! It's a great way to stay current with GitHub's style recommendations.

One small issue that I've noticed is that the ApplicationRecord subclass cop runs on application_record.rb in addition to other models. This is what I'm getting:

app/models/application_record.rb:2:27: C: Models should subclass from ApplicationRecord
class ApplicationRecord < ActiveRecord::Base
                          ^^^^^^^^^^^^^^^^^^

Changing this results in a circular dependency issue, of course. Just wanted to point it out. 😄

The current purpose of github-rubocop

The current purpose of this Repository from the README:

This repository provides recommended RuboCop configuration and additional Cops for use on GitHub open source and internal Ruby projects.

What does that include?

I think that includes:

• A. Baseline rules (config/default.yml) for upstream rubocop, rubocop-rails, rubocop-performance, etc. that is a consistent starting place for GitHub-maintained projects. Individual projects/teams are empowered to customize them to their preference.
• B. GitHub-specific cops that are specific to "GitHub open source and internal Ruby projects". For example:
  • Cops that correspond to specific internal APIs, or Github-specific monkeypatches/behavior on specific projects.
  • Cops that are relevant to all GitHub-managed projects (but not any project anywhere)

What doesn't that include?

I think that does not include:

• C. Cops that would be applicable to all projects within and outside of GitHub.

Why distinguish GitHub from Everyone?

I think we will benefit the broader community/ecosystem if we make Cops that are applicable to all projects available without having to pull in Rules and Cops that are only narrowly applicable to us.

I think we also will benefit ourselves by sending a stronger signal that we believe those Cops are good for everyone and invite community discussion and participation.

Where will the Cops go instead?

I think this workflow matches how other upstream contributions are managed internally at GitHub:

1. Propose the change upstream in the core open source project, where they have maximal visibility and discussion/feedback.
2. Patch them provisionally into GitHub's project(s), for immediate benefit. That could include this repository, or the proposer's Ruby project.
3. ...if the proposal is accepted, remove our patch and pull the updated upstream
4. ...if the proposal is not accepted, we usually learn something about our proposal, and we then choose whether to maintain the patch ourselves, discard it, or identify a better open-source home for it.

What is the current disposition of Cops?

I definitely think this is more a molehill than a a mountain. And I think making the disposition of cops exemplary will help model the outcome we want to see (👪=Everyone, 🐙=GitHub, 🚝=GitHub's Monolith, ❌=redundant/removable)

• 🐙 insecure_hash_algorithm.rb: 🐙 All-GitHub. FIPS compliance
• 👪 / ❌ rails_application_record.rb: Everyone (duplicate in rubocop-rails)
• 🚝 rails_controller_render_action_symbol.rb: GitHub Monolith (performance-related)
• 🚝 rails_controller_render_literal.rb GitHub Monolith (performance-related)
• 🚝 / 👪 rails_controller_render_paths_exist.rb: GitHub Monolith (performance-related) or Rubocop-Rails?
• 👪 rails_controller_render_shorthand.rb: Everyone (I think I’ve seen this in the community, but now can’t find it)
• 👪 / ❌ rails_render_inline.rb: Everyone (duplicate in rubocop-rails)
• 🚝 rails_render_object_collection.rb: GitHub Monolith?
• 🚝 rails_view_render_literal.rb: GitHub Monolith?
• 🚝 / 👪 rails_view_render_paths_exist.rb: GitHub Monolith? Or Rubocop-Rails.
• 🚝 rails_view_render_shorthand.rb: GitHub Monolith (performance-related)

That adds up to:

• 🐙 1 cop that’s relevant to all GitHub projects. Because of our Security requirements (though reasonably applicable to everyone)
• 🚝 5 cops that are only relevant to the Monolith. Mainly because of performance patches we’ve done to render that I don’t think are widely relevant (e.g. Rails flexible development vs GitHub disciplined performance)
• 🚝 / 👪: 2 cops I’m not sure if they’re relevant to everyone, I’m leaning towards Monolith.
• 👪: 6 cops that are relevant to everyone, mainly Accessibility
• ❌: 2 cops that have been upstreamed that we can remove

The Proposal

• Document/clarify in README how to self-triage proposed changes
• Create an internal service catalog entry for better internal discovery and communications
• Namespace update the existing Cops so that their intended target is clearer, and/or move them to a different more appropriate project.
• Remove redundant/removable cops
  • #123

When you pass a variable instead of an explicit hash to render, Rubocop complains that "render must be used with a string literal or an instance of a Class".

Linter complains:

render("some/template", locals: hash_of_locals)

Linter does not complain:

render("some/template", locals: {**hash_of_locals})

Assuming there is no particular issue with passing in a hash variable to locals, it seems like maybe there is just a parsing issue here? Or...if that is in fact an issue in latest Rails, maybe the message just needs updating.

As your styleguide recommends

class TestClass
  # bad
  class << self
    def first_method
      # body omitted
    end

    def second_method_etc
      # body omitted
    end
  end

  # good
  class << self
    attr_accessor :per_page
    alias_method :nwo, :find_by_name_with_owner
  end

  def self.first_method
    # body omitted
  end

  def self.second_method_etc
    # body omitted
  end
end

I was wondering why. If it's just a matter of taste for you: fine, please write this in the styleguide as well.

It isn't obvious here why you are preferring this because especially in cases with many class methods the block and therefore also the additional indention give a good visual hint that methods are encapsulated and belong together. They share a similar context and blocks are the usual way of creating contexts.

Besides from that it can also help to avoid to have class methods spread over a file. It is easier (at least for people I know) to have class methods together in one place than having a bunch of instance methods in between.

The Eigenclass context also makes sense in your example as per_page and first_method both will create class methods. But in your example just attr_accessor and alias_method are using the Eigenclass context whereas first_method and second_method_etc give an 'defined from the outside' feeling.

These points makes it more understandable for me to follow bbatsovs / rubocops styleguide which says

  # Also possible and convenient when you
  # have to define many class methods.
  class << self
    def first_method
      # body omitted
    end

    def second_method_etc
      # body omitted
    end
  end

Change in rubocop 0.78.0: https://github.com/rubocop-hq/rubocop/blob/master/CHANGELOG.md#changes-2

Needs update here

In #132 we linked each styleguide rule to its corresponding RuboCop rule(s).

RuboCop has functionality where a StyleGuide: <link> config for each cop will show the link to the styleguide when there's a violation or a user just wants to know what cops are enabled. We haven't added all of these cross-references yet, and we should finish off.

For each of the rules in the styleguide, add StyleGuide: under its cop name in config/default.yml. If it doesn't have a config, add the full stanza. For example:

# This rule is specifically disabled, but we still want it to have a styleguide link.
Layout/LineLength:
  Enabled: false
  StyleGuide: https://github.com/github/rubocop-github/blob/main/STYLEGUIDE.md#line-length

# Pretend this rule didn't exist in the config file before, as an example. Since it's a RuboCop default cop and doesn't need any special configuration. But, we still want a styleguide link, so add the whole rule name.
Layout/EmptyLineBetweenDefs:
  StyleGuide: https://github.com/github/rubocop-github/blob/main/STYLEGUIDE.md#empty-lines-def