Handle schema_dump_path being unavailable in older Rails versions
#264
Conversation
andyw8
changed the title
schema_dump_path being unavailable in older Rails versions
andyw8
force-pushed
the
andyw8/schema_dump_path-not-in-rails-6
branch
from
b58cd72 to
77150d4
Compare
| @@ -26,7 +26,7 @@ class Server | |||
|
|
|||
| extend T::Sig | |||
|
|
|||
| sig { params(model_name: String).returns(T.nilable(T::Hash[Symbol, T.untyped])) } | |||
| sig { params(model_name: String).returns(T::Hash[Symbol, T.untyped]) } | |||
There was a problem hiding this comment.
I realised that we always return a hash.
andyw8
force-pushed
the
andyw8/schema_dump_path-not-in-rails-6
branch
from
77150d4 to
739fba6
Compare
| require "test_helper" | ||
| require "ruby_lsp/ruby_lsp_rails/server" | ||
|
|
||
| class ServerTest < ActiveSupport::TestCase |
There was a problem hiding this comment.
@vinistock since the server is in a different process, this needs to be tested differently from the other aspects in runner_client_test.rb. But I think it would make sense to move some of the other tests to here too, and leave just #model returns information for the requested model as an integration test?
There was a problem hiding this comment.
Pushed a commit to show how that would look.
There was a problem hiding this comment.
I think this is fine, but let's put the server in an instance variable instead of doing Server.new on every test.
There was a problem hiding this comment.
I think this is fine, but let's put the server in an instance variable instead of doing Server.new on every test.
andyw8
changed the title
schema_dump_path being unavailable in older Rails versionsschema_dump_path being unavailable in older Rails versions
andyw8
force-pushed
the
andyw8/schema_dump_path-not-in-rails-6
branch
from
1469313 to
4aadd0b
Compare
| }, | ||
| } | ||
|
|
||
| if ActiveRecord::Tasks::DatabaseTasks.respond_to?(:schema_dump_path) |
There was a problem hiding this comment.
It's possible we can make this also work in Rails 6 and earlier, if someone wants to open a PR.
andyw8
force-pushed
the
andyw8/schema_dump_path-not-in-rails-6
branch
from
4aadd0b to
2749956
Compare
| test "returns nil if model doesn't exist" do | ||
| assert_nil @client.model("Foo") | ||
| end | ||
|
|
||
| test "returns nil if class is not a model" do | ||
| assert_nil @client.model("Time") | ||
| end | ||
|
|
||
| test "returns nil if class is an abstract model" do | ||
| assert_nil @client.model("ApplicationRecord") | ||
| end |
There was a problem hiding this comment.
Good point, restored.
There was a problem hiding this comment.
Did you restore all of them? I'm only seeing the abstract one.
There was a problem hiding this comment.
Only one, to validate the nil return. I don't think it's necessary to re-verify the behaviour already covered in server_test.rb?
There was a problem hiding this comment.
Ah, right. It's covered in the new test. 👍
| require "test_helper" | ||
| require "ruby_lsp/ruby_lsp_rails/server" | ||
|
|
||
| class ServerTest < ActiveSupport::TestCase |
There was a problem hiding this comment.
I think this is fine, but let's put the server in an instance variable instead of doing Server.new on every test.
| require "test_helper" | ||
| require "ruby_lsp/ruby_lsp_rails/server" | ||
|
|
||
| class ServerTest < ActiveSupport::TestCase |
There was a problem hiding this comment.
I think this is fine, but let's put the server in an instance variable instead of doing Server.new on every test.
andyw8
force-pushed
the
andyw8/schema_dump_path-not-in-rails-6
branch
from
562cdf1 to
efe472b
Compare
Closes #260. For older Rails versions we will show the fields but won't provide a link to the schema file.