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

Add Rails Runner client #250

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Add Rails Runner client #250

merged 1 commit into from
Feb 13, 2024

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Feb 9, 2024

This PR is part of a migration towards communicating with the Ruby LSP server over a pipe, rather than HTTP. It was copied out of the branch andyw8/implement-lsp-runner for easier review.

I have tophatted the above branch against Code DB and Core. On Core, it correctly jumps the correct one of the two SQL structure files.

Previous discussion on prototype.


sig { params(name: String).returns(T.nilable(T::Hash[Symbol, T.untyped])) }
def model(name)
make_request("model", name: name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The param was called models in the Rack app, which matched up with the RESTful route name, but here I think model is more appropriate.

@andyw8 andyw8 force-pushed the andyw8/add-runner-client branch 2 times, most recently from 54cca6f to 72f4c60 Compare February 9, 2024 19:21
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@andyw8 andyw8 marked this pull request as ready for review February 9, 2024 21:38
@andyw8 andyw8 requested a review from a team as a code owner February 9, 2024 21:38
@andyw8
Copy link
Contributor Author

andyw8 commented Feb 9, 2024

This still needs a little clean-up but feedback is welcome.

lib/ruby_lsp/ruby_lsp_rails/runner_client.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/ruby_lsp_rails/runner_client.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/ruby_lsp_rails/server.rb Show resolved Hide resolved
lib/ruby_lsp/ruby_lsp_rails/server_start.rb Outdated Show resolved Hide resolved
test/ruby_lsp_rails/runner_client_test.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/ruby_lsp_rails/runner_client.rb Outdated Show resolved Hide resolved
@andyw8 andyw8 force-pushed the andyw8/add-runner-client branch 2 times, most recently from 56f75b8 to afc4f28 Compare February 12, 2024 19:26
@andyw8 andyw8 changed the title WIP: Add Rails Runner client Add Rails Runner client Feb 12, 2024
@andyw8 andyw8 force-pushed the andyw8/add-runner-client branch from afc4f28 to a306d81 Compare February 12, 2024 19:31
end

sig { returns(T::Boolean) }
def stopped?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid the test having to poke inside with instance_var_get.

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

Awesome

end
end

RubyLsp::Rails::Server.new.start if ARGV.first == "start"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this argument? This script is always invoked from the runner, so would there be a case where we wouldn't want to start it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can revisit this, but currently without it the check_docs task will hang because it requires each file.

Copy link
Member

Choose a reason for hiding this comment

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

I see. We can probably ignore this file in check_docs.

@egiurleo egiurleo removed their request for review February 12, 2024 21:12
@andyw8 andyw8 merged commit a600e02 into main Feb 13, 2024
54 checks passed
@andyw8 andyw8 deleted the andyw8/add-runner-client branch February 13, 2024 14:53
}
end

schema_file = ActiveRecord::Tasks::DatabaseTasks.schema_dump_path(const.connection.pool.db_config)
Copy link

@D-system D-system Feb 19, 2024

Choose a reason for hiding this comment

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

This line break the compatibility with ActiveRecord 6

schema_dump_path is not present in ActiveRecord 6.1: https://github.com/rails/rails/blob/v6.1.7.6/activerecord/lib/active_record/tasks/database_tasks.rb

schema_dump_path is available from 7.0: https://github.com/rails/rails/blob/v7.0.0/activerecord/lib/active_record/tasks/database_tasks.rb#L448

[EDIT: open an issue #260]

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.

3 participants