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

standard-1-37-0 #4

Merged
merged 8 commits into from
Dec 4, 2024
Merged

Conversation

milesstanfield
Copy link
Collaborator

The current version of this engine is using standard 1.4.0 which was released 3 years ago. The lack of a newer standard version is encouraging some developers into Running Standard's rules via RuboCop instead of using it at all.

This pr updates the engine into using standard 1.37.0 (released just 5 months ago) which is the last version that works on ruby 2.x.

gem "pry", require: false
gem "safe_yaml"
gem "standard", "~> 1.4", require: false
gem "standard", "1.37.0", require: false
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hardlocked without twiddle wakka to emphasize this is the last version before ruby 3 is a minimum requirement for standard

@@ -2,10 +2,10 @@ source "https://rubygems.org"

gem "activesupport", require: false
gem "mry", "~> 0.52.0", require: false
gem "parser", "~> 3.0.2"
gem "parser", "~> 3.3.0"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required in order to satisfy gem dependencies

@@ -1,5 +1,6 @@
require "spec_helper"
require "cc/engine/issue"
require "ostruct"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This spec and the one below was failing after the gem updates due to needing OpenStruct explicitly required now. OpenStruct is not used in the engine itself, only in specs. Therefore adding these requires here is an appropriate fix

uninitialized constant CC::Engine::OpenStruct

@@ -30,7 +30,7 @@ module CC::Engine
expect { config.validate }.to output(<<~TXT).to_stderr
The `Style/TrailingComma` cop has been removed. Please use `Style/TrailingCommaInArguments`, `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)
unrecognized cop Style/TrailingComma found in .rubocop.yml
unrecognized cop or department Style/TrailingComma found in .rubocop.yml
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On master without any changes at all, this is failing. Seems that Style/TrailingComma modified its output and rake docs:scrape has differences in it than what it once was

@@ -3,3 +3,4 @@ RuboCop::Cop::Lint::Syntax
RuboCop::Cop::Migration::DepartmentName
RuboCop::Cop::Style::ConditionalAssignment
RuboCop::Cop::Style::DoubleCopDisableDirective
RuboCop::Cop::Style::BlockDelimiters
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, on master without any changes, this too is also failing a spec (even after running rake docs:scrape). Added here to fix

"#{cop.name} has no content ..."

@@ -4,6 +4,9 @@
module StandardRunner
def self.included(example_group)
example_group.include FilesystemHelpers
example_group.before do
allow_any_instance_of(RuboCop::AST::ProcessedSource).to receive(:registry).and_return(registry)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after the updates (due to rubocop version change), we see a STDERR after 3 specs run

An error occurred while Lint/MissingCopEnableDirective cop was inspecting /tmp/.../test.rb
An error occurred while Lint/UselessAssignment cop was inspecting /tmp/.../test.rb
An error occurred while Style/RedundantReturn cop was inspecting /tmp/.../test.rb

If you add a debug: true kwarg to lib/cc/engine/source_file#rubocop_team instance of RuboCop::Cop::Team.new instantiation, we get a detailed backrace (analogous to using rubocop -d cli + flag) which says

undefined method 'disabled' for nil:NilClass
  /home/milesstanfield/.rbenv/versions/3.3.6/lib/ruby/gems/3.3.0/gems/rubocop-1.68.0/lib/rubocop/comment_config.rb:116:in `inject_disabled_cops_directives'

and points to this line of code in the rubocop gem. A github issue was opened about this and it appears to only be spec-related. There was a more involved solution proposed but generally the idea is we need to stub this registry method call value on the RuboCop::AST::ProcessedSource instance.

That is what i've done here by borrowing from the code suggested as a fix in that github issue using CopHelper#registry found here

@@ -21,10 +24,22 @@ def issues(output = @engine_output)
end

def run_engine(config = nil)
@config = config
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set as an instance variable now so if config arg was passed into this (which it is in at least one spec in this repo) then we use that when generating the registry value instead of a blank config object (see below)

@milesstanfield milesstanfield marked this pull request as ready for review November 23, 2024 13:02
@milesstanfield
Copy link
Collaborator Author

@jakeonfire I dont have privs to request reviews so im @'ing you to ask could you please take a look at this pr? Thanks!

@jakeonfire
Copy link
Owner

we've switched to using standard through rubocop (for other reasons) 😬
would you like to be a contributor for this repo? that seems easier since i'm a bit out-of-touch with this project. i've invited you 🙂

@@ -9,7 +9,7 @@ RUN adduser -u 9000 -g 9000 -D app
COPY Gemfile Gemfile.lock /usr/src/app/

RUN apk add --update build-base && \
gem install bundler && \
gem install bundler -v 2.4.22 && \
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes issue running make image

The last version of bundler (>= 0) to support your Ruby & RubyGems was 2.4.22. Try installing it with `gem install bundler -v 2.4.22

@@ -11,7 +11,12 @@ You can find some basic setup instructions and links to the Standard OSS project
### Installation

1. If you haven't already, [install the Code Climate CLI](https://github.com/codeclimate/codeclimate).
2. Add the engine and enable it in your `.codeclimate.yml` file.
2. Enable the engine by adding the following under `plugins` in your `.codeclimate.yaml`:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adjusting README to mimic the rubocop engine instructions (which this repo was apparently made from since it has some of the original commits in it). Modified slightly to use standard as the engine name below not rubocop seen here

This also sets it up nicely for when the next standard channel (> 1.37.0) is released which will require a minimum ruby version of >= 3. I suspect given the default implementation is to either not set the channel explicitly in the .codeclimate.yml file or set it to "latest", we may (at that time in the future) see some regression when we move to that next version.

The reason I think having this setup in the README helps is we can add a channel: line to it with some comments about which explicit channel to use if you are running ruby < 3 since "latest" will be at that point running the standard version that requires it

@milesstanfield milesstanfield merged commit 856c49e into jakeonfire:master Dec 4, 2024
@milesstanfield milesstanfield deleted the standard-1-37-0 branch December 4, 2024 13:14
@milesstanfield milesstanfield restored the standard-1-37-0 branch December 4, 2024 13:33
@milesstanfield
Copy link
Collaborator Author

@jakeonfire thanks! One last thing, i have no visibility into the .circleci ci/cd flow and can only confirm by it's absence of channel: "standard-1-37-0" working that the .circleci pipeline either did not run or ran and encountered an error.

Could you either: look at what failed and either fix it or grant me access to it so i can fix it

OR

grant me access to the gcloud docker project so i can push the image manually

@jakeonfire
Copy link
Owner

@milesstanfield unfortunately that's carryover from the rubocop version and i have no access to that infra either 😬

@jakeonfire
Copy link
Owner

jakeonfire commented Dec 4, 2024

if it were working, i would expect to see GitHub Actions artifacts like these in PRs.
Screenshot 2024-12-04 at 10 39 09 AM
Screenshot 2024-12-04 at 10 39 49 AM
these are from codeclimate#374

@jakeonfire
Copy link
Owner

jakeonfire commented Dec 4, 2024

also, i have gotten less and less response from my attempts to contact codeclimate over the last few years. not sure what's going on there, but just a heads up. we canceled our subscription and are looking for a new solution. they do have a Slack workspace that i used to use.

@milesstanfield
Copy link
Collaborator Author

@jakeonfire Ok well at least that answers all my questions. I think unfortunately we will lean more into the rubocop approach then as well and eject from this endeavor as well. Thanks for all your help and communication 👍

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.

2 participants