Skip to content

Change how we warn about server not running #88

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

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Jun 22, 2023

There are legitimate cases why someone may want to edit code without the server running, so we want to avoid over-notifying. In this PR, we are changing the behaviour so that we output a notice in the logs, rather than showing an error dialog.

We should also update the README to explain how some features only available if the server is running, but since there is only currently one feature (Hover), it probably makes more sense to do that after Code Lens is added in #67.

Paired on with @vinistock

@@ -13,8 +13,6 @@
require "ruby_lsp/internal"
require "ruby_lsp/ruby_lsp_rails/extension"

$VERBOSE = nil unless ENV["VERBOSE"] || ENV["CI"]
Copy link
Contributor Author

@andyw8 andyw8 Jun 22, 2023

Choose a reason for hiding this comment

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

This was added to reduce noise in test outout, but it was preventing us from asserting against stderr.

@@ -22,23 +22,6 @@ class RailsClientTest < ActiveSupport::TestCase
assert_equal(expected_response, RailsClient.instance.model("User"))
end

test "raises during instantiation if app_uri file doesn't exist" do
Copy link
Contributor Author

@andyw8 andyw8 Jun 22, 2023

Choose a reason for hiding this comment

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

No longer relevant since we don't do anything during instantiation.

@@ -13,7 +13,6 @@ class Extension < ::RubyLsp::Extension

sig { override.void }
def activate
# Must be the last statement in activate since it raises to display a notification for the user
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer relevant since we don't raise.

@andyw8 andyw8 marked this pull request as ready for review June 23, 2023 15:38
@andyw8 andyw8 requested a review from a team as a code owner June 23, 2023 15:38
Copy link
Contributor

@egiurleo egiurleo left a comment

Choose a reason for hiding this comment

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

Nice! I think this will be a much better experience.

@andyw8 andyw8 merged commit b9ba758 into main Jun 23, 2023
@andyw8 andyw8 deleted the andyw8/change-how-we-warn-about-server-not-running branch June 23, 2023 18:09
@shopify-shipit shopify-shipit bot temporarily deployed to production June 30, 2023 19:47 Inactive
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