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

Get the build green for modern ruby and rack and rubocop-rspec #939

Conversation

h-lame
Copy link
Contributor

@h-lame h-lame commented Jul 5, 2024

Separated out from #938 this PR attempts to get the master build to green from a fresh checkout.

These days you're probably on ruby 3.x and will get rack 3.x and rubocop-rspec 3.x too. These new versions all cause problems with the code in master, so the build won't pass.

  1. Update HTTP status codes and associated symbols rack/rack#2137 - Handle the fact that the values in Rack::Utils::SYMBOL_TO_STATUS_CODE are now different and this causes the dummy rails app we use for testing to not load and this blocks the entire suite from running. The solution is to use Rack::Utils.status_code which has a workaround for the old names.
  2. Make env['rack.input'] optional. rack/rack#2018 - Handle the fact that rack.input is now optional in the rack env. The solution is to use safe navigation to call the methods we wanted.
  3. Remove extracted cops in Capybara, FactoryBot and Rails departments. rubocop/rubocop-rspec#1848 - Introduce rubocop-rspec_rails as the cops it provides were extracted from rubocop-rspec and our config in rubocop_todo.yml referenced them and rubocop doesn't like config for cops it doesn't know about so that blocked the build.
  4. Fix the one rubocop offence that being able to run the build again let us see.

As part of rack/rack#2137 (released in Rack 3) Rack
changed the values of `Rack::Utils::SYMBOL_TO_STATUS_CODE` so asking for
`:unprocessable_entity` now returns `nil`. The guidance has always been to use
`Rack::Util.status_code` (present in the API since Rack 1.1) so let's just do
that.
Ever since rack/rack#2018 `rack.input` has been
optional so we can't assume it's there and call `read` or `rewind` on it in
`Apipie::Extractor::Recorder`. Using safe navigation to call these methods
seems like the simplest approach because we want to look for
`rack.request.form_hash` instead in both the situation where `rack.input` is
missing, or it's empty.
In rubocop/rubocop-rspec#1848 rubocop-rspec extracted
some cops to separate gems, and because we referenced one of them in our
rubocop_todo.yml the whole rubocop process wouldn't run because it doesn't like
config for cops it doesn't know about. We introducing the missing gem,
`rubocop-rspec_rails`, and fix the name in the config to let us run rubocop
again.

There is one infraction that exists in the source so we fix that to get us to a
green rubocop run.
Copy link
Collaborator

@mathieujobin mathieujobin left a comment

Choose a reason for hiding this comment

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

👍🏽 LGTM

@@ -38,6 +38,7 @@ Gem::Specification.new do |s|
s.add_development_dependency 'rubocop-rails'
s.add_development_dependency 'rubocop-rspec'
s.add_development_dependency 'rubocop-performance'
s.add_development_dependency 'rubocop-rspec_rails'
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you try adding if RUBY_VERSION >= "2.7" please?

or take out this line for now, ... if you can

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that, but the problem will probably then be that ruby 2.6 rubocop will complain about RSpecRails/InferredSpecType being a cop it doesn't understand whereas ruby 2.7+ rubocop complains about RSpec/Rails/InferredSpecType. Can we put conditional rubocop_todo by ruby version?

In another thread I think you suggested someone had dropped ruby 2.6 / rails 5 support - should we make that official?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll push up a commit with the if version comparison so we can test that theory though

It's only available for 2.7+ and we still run this build on 2.6.
@mathieujobin
Copy link
Collaborator

@h-lame The condition is cool, we just need to add a rubocop exclusion for it, I think its fine, I will probably remove support for ruby 2.6 soon anyway, not much of a point anymore. bit in the mean time. its good.

mathieujobin added a commit that referenced this pull request Jul 9, 2024
@mathieujobin mathieujobin merged commit 89e7ded into Apipie:master Jul 9, 2024
24 of 25 checks passed
@h-lame h-lame deleted the get-the-build-green-for-modern-ruby-and-rack-and-rubocop branch July 9, 2024 16:34
@h-lame
Copy link
Contributor Author

h-lame commented Jul 9, 2024

Thanks! Reading up on the now failing rubocop complaint about using RUBY_VERSION in the gem spec, sounds like the preferred option is moving all the add_development_dependency things to Gemfile (see rubygems/rubygems#7799 (comment)) - any interest in me doing that?

@mathieujobin
Copy link
Collaborator

@h-lame No thanks, the condition is for ruby 2.6, which we'll probably remove support for. so that exclusion will go away anyway soon.

about the comment you linked on rubygems, I don't see how rubygems will be able to deprecate add_development_dependency, I guess they can all be moved to the Gemfile instead but ... I think a lot of gem owners will not be happy about that.

thanks for your contribution to Apipie

@h-lame
Copy link
Contributor Author

h-lame commented Jul 14, 2024

about the comment you linked on rubygems, I don't see how rubygems will be able to deprecate add_development_dependency, I guess they can all be moved to the Gemfile instead but ... I think a lot of gem owners will not be happy about that.

Yeah, I agree, seems like a pretty big breaking change to leave in a comment on an unrelated issue.

thanks for your contribution to Apipie

No worries! Thanks for accepting it.

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