Skip to content

Conversation

Smittttty
Copy link
Contributor

@Smittttty Smittttty commented Jun 14, 2022

Motivation

PaymentService has an engine thats named Internal. With the latest version of ruby-lsp, PaymentService fails to load the Internal component. Related Buildkite.

586eccf was the only change that seemed like it could have caused this. Renaming the internal.rb file resolves the issue, as does moving the file to not be in the top-level /lib folder.

Implementation

Moved internal.rb to be under the ruby_lsp directory.

@Smittttty Smittttty added the bug Something isn't working label Jun 14, 2022
@Smittttty Smittttty requested a review from a team June 14, 2022 19:43
@Smittttty Smittttty removed the bug Something isn't working label Jun 14, 2022
@st0012
Copy link
Member

st0012 commented Jun 14, 2022

I think maybe the problem is that there shouldn't be any non-ruby-lsp.rb file located at the top-level /lib. Even if we changed it to requires.rb, it can still be loaded with require "requires", which may be some other gems' name. So the better solution to me is to place the internal.rb under the ruby-lsp namespace.

@paracycle
Copy link
Member

Yeah 100% agreed with what @st0012 wrote above. We should be moving the require to inside the ruby-lsp folder.

@Smittttty Smittttty force-pushed the rename-internal-to-requires branch from 31c065e to 8031ff2 Compare June 14, 2022 21:39
@Smittttty Smittttty changed the title Renamed internal.rb -> requires.rb Moved internal.rb to be under the ruby_lsp directory Jun 14, 2022
@Smittttty Smittttty requested a review from paracycle June 14, 2022 21:40
@Smittttty Smittttty force-pushed the rename-internal-to-requires branch from 8031ff2 to e826b68 Compare June 14, 2022 21:43
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Thanks and sorry that this broke your workflow!

@paracycle paracycle merged commit c3371bb into main Jun 14, 2022
@paracycle paracycle deleted the rename-internal-to-requires branch June 14, 2022 21:45
@Smittttty
Copy link
Contributor Author

Thanks and sorry that this broke your workflow!

No problem, thanks for the quick review! ❤️

@shopify-shipit shopify-shipit bot temporarily deployed to production June 15, 2022 15:46 Inactive
andyw8 referenced this pull request in andyw8/ruby-lsp Mar 2, 2024
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