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

Watch for YAML file changes for fixtures #557

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Jan 17, 2025

This is to support Shopify/tapioca#2150 but will probably be useful for other upcoming features.

@andyw8 andyw8 force-pushed the andyw8/watch-for-yaml-file-changes branch from 17812c5 to ff2cdb7 Compare January 17, 2025 15:26
@andyw8 andyw8 added the chore Chore task label Jan 17, 2025
@andyw8 andyw8 changed the title WIP: Watch for YAML file changes Watch for YAML file changes Jan 17, 2025
@andyw8 andyw8 marked this pull request as ready for review January 17, 2025 15:29
@andyw8 andyw8 requested a review from a team as a code owner January 17, 2025 15:29
@andyw8
Copy link
Contributor Author

andyw8 commented Jan 17, 2025

@vinistock I'm unsure how we'd go about writing a test for this, any thoughts?

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.

I'm not sure what we could test. There's not a ton of logic, it's essentially just sending configuration to the client.

lib/ruby_lsp/ruby_lsp_rails/addon.rb Outdated Show resolved Hide resolved
@andyw8 andyw8 force-pushed the andyw8/watch-for-yaml-file-changes branch 3 times, most recently from ae0e434 to 08eaf66 Compare January 17, 2025 16:17
@@ -3,6 +3,7 @@

require "json"
require "open3"
require "rails/test_help" # to determine the fixture_paths
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 have a slight concern that this might alter the behaviour in some way.

https://github.com/rails/rails/blob/main/railties/lib/rails/test_help.rb

Copy link
Member

Choose a reason for hiding this comment

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

Can't we require a smaller subset of things? Do we need anything beyond active_support/test_case? Or can we match the logic in Rails and avoid requiring anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use only the minimum parts.

@andyw8 andyw8 changed the title Watch for YAML file changes Watch for YAML file changes for fixtures Jan 17, 2025
@andyw8 andyw8 force-pushed the andyw8/watch-for-yaml-file-changes branch 3 times, most recently from 7ed3715 to f7643cd Compare January 20, 2025 21:59
ActiveSupport.on_load(:active_support_test_case) do
include ActiveRecord::TestFixtures

fixture_paths << "#{Rails.root}/test/fixtures/"
Copy link
Member

Choose a reason for hiding this comment

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

Where do people normally configure their fixture_paths?

I just realized that, if people do it in test/test_helper.rb or in environments/test.rb, we are probably going to miss it anyway.

I may have set you on the wrong path here, I'm not sure we can accurately detect which fixture paths were configured.

Also, the server.rb file is loaded after the Rails app is fully initialized, so any initializers would have already run.

@andyw8 andyw8 force-pushed the andyw8/watch-for-yaml-file-changes branch from 140761e to 7f90b1d Compare January 21, 2025 16:21
@andyw8 andyw8 force-pushed the andyw8/watch-for-yaml-file-changes branch from 7f90b1d to e712ffd Compare January 21, 2025 16:38
@andyw8 andyw8 merged commit b779129 into main Jan 21, 2025
28 checks passed
@andyw8 andyw8 deleted the andyw8/watch-for-yaml-file-changes branch January 21, 2025 17:41
@vinistock vinistock added enhancement New feature or request and removed chore Chore task labels Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants