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

WIP Update Rubocop #33

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

WIP Update Rubocop #33

wants to merge 17 commits into from

Conversation

iurev
Copy link
Contributor

@iurev iurev commented May 8, 2023

Important updates:

  • dropped support for ruby 2.3 because anycable gem supports ruby >= 2.7 (which is also non-secure already)
  • moved dev gems from gemspec to Gemfile
  • added rubocop workflow to GitHub Actions
  • manually fixed some rubocop warnings

Future plans:

  • finish rubocop_todo.yml. It will require me to refactor some tests and dive deeper into the project, which is a good thing.

Copy link
Member

@Envek Envek left a comment

Choose a reason for hiding this comment

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

Let's try rubocop --autocorrect maybe? 😄

.rubocop.yml Outdated Show resolved Hide resolved
@Envek
Copy link
Member

Envek commented May 9, 2023

dropped support for ruby 2.3 because anycable gem supports ruby >= 2.7 (which is also non-secure already)

I'm fine with that, but then we probably need to bump anycable version in gemspec to one that started to require Ruby 2.7.

In general it is better to support older Rubies as long as possible, maybe with help from ruby-next (there are a lot applications on old unmaintained Rubies in the wild)

moved dev gems from gemspec to Gemfile

👍

@iurev
Copy link
Contributor Author

iurev commented May 9, 2023

I'm fine with that, but then we probably need to bump anycable version in gemspec to one that started to require Ruby 2.7.

In general it is better to support older Rubies as long as possible, maybe with help from ruby-next (there are a lot applications on old unmaintained Rubies in the wild)

@Envek, AnyCable supports ruby >= 2.7. I think in this case supporting the same ruby version is enough. Link to AnyCable Requirements

@iurev iurev marked this pull request as ready for review May 15, 2023 16:11
@@ -1,7 +1,7 @@
# frozen_string_literal: true

RSpec.describe GraphQL::AnyCable do
subject do
subject(:execute_query) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Envek, I gave subjects names based on the spec NamedSubject. What do you think about this? I am especially worried about the possibility that I gave not-good-enough names for these variables.

socket = double("Socket", istate: AnyCable::Socket::State.new({}))
connection = double("Connection", anycable_socket: socket)
double("Channel", id: "legacy_id", params: { "channelId" => "legacy_id" }, stream_from: nil, connection: connection)
socket = instance_double(AnyCable::Socket, istate: AnyCable::Socket::State.new({}))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Envek, I have changed double to instance_double and have passed classes instead of strings. Do you think this will improve the readability? The tests pass successfully, but I am unsure if using these classes and modules is 100% correct.

Because of these changes, I had to add the method id to the class FakeConnection::Channel. Is that okay?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it is fine. Thanks!

end

let(:object) do
double("Post", id: 1, title: "Broadcasting…", actions: %w[Edit Delete])
double(Post, id: 1, title: "Broadcasting…", actions: %w[Edit Delete]) # rubocop:disable RSpec/VerifiedDoubles
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Envek, I could not use instance_double here because instances of GraphQL::Schema::Object don't have methods.

@@ -3,6 +3,8 @@
require "integration_helper"

RSpec.describe "broadcastable subscriptions" do
subject(:execute_request) { handler.handle(:command, request) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Envek, I cannot understand where the variable request comes from. Can you show me that?

Copy link
Member

Choose a reason for hiding this comment

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

I believe it is this one from anycable gem (it is included in the integration_helper).

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