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

Make auto require more data model aware? #625

Open
LordOfSpelunky opened this issue May 20, 2024 · 4 comments
Open

Make auto require more data model aware? #625

LordOfSpelunky opened this issue May 20, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@LordOfSpelunky
Copy link

I'm experimenting with using Luau LSP internally at Roblox. Auto requires are fantastic, but we have a common pattern of the following:

local Plugin = script:FindFirstAncestor("PluginName")

local Script = require(Plugin.Src.Script)

Unfortunately, it looks like Luau LSP will auto-require without the Plugin part, and will just script.Parent etc, which we don't want.

I think that Luau LSP should (optionally?) use variables if they exist and statically point to the strict data model instance. That is, if we have some require already such that Luau LSP already knows where to put stuff, and it sees that the equivalent to script.Parent.Parent exists in the form of Plugin, it should use that.

I think another problem with trying to use auto requires exclusively for us is that I'm not sure Luau LSP will actually split itself off from the Plugin variable, but instead might try to put itself at the beginning. Would be cool to also be able to tell it to place the first require wherever it can find a strict data model instance?

@JohnnyMorganz
Copy link
Owner

Sounds reasonable to support to me! We can definitely be more clever in our auto require logic, the current implementation is quite rudimentary.

I'm not sure Luau LSP will actually split itself off from the Plugin variable, but instead might try to put itself at the beginning.

I think this should follow automatically by implementing the first point - we should add the require after the fixed variable.

@JohnnyMorganz JohnnyMorganz added the enhancement New feature or request label May 20, 2024
@LordOfSpelunky
Copy link
Author

Cool 😃

As a side note, I forgot to mention that ideally it puts some whitespace around itself too like the GetService option

@JohnnyMorganz
Copy link
Owner

Do you mean whitespace between the fixed variable (Plugin) and the block of requires (Script etc.?)
Also do you reckon this should follow the existing config completion.imports.separateGroupsWithLine? Or whitespace between the variables should be a separate config

@LordOfSpelunky
Copy link
Author

Yeah basically I mean that I want to make sure that in the following code:

local Plugin = script:FindFirstAncestor("PluginName")

local function doinStuff()

...that the first require gets placed like:

local Plugin = script:FindFirstAncestor("PluginName")

local Library = require(Plugin.Src.Library)

local function doinStuff()

No idea WRT separateGroupsWithLine. I think it makes sense to put it under there on first blush though.

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

No branches or pull requests

2 participants