-
Notifications
You must be signed in to change notification settings - Fork 34
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
lsp/templating: gracefully handle unknown root #1171
base: main
Are you sure you want to change the base?
Conversation
This also disables templating for files in the root. Fixes StyraInc#1164 Related to StyraInc#1141, but this needs more work Signed-off-by: Charlie Egan <[email protected]>
// if there are no parsed modules in the cache, then there is | ||
// no need to run the aggregate report. This can happen if the | ||
// server is very slow to start up. | ||
if len(l.cache.GetAllModules()) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated, but is something that I noticed when working with empty files, a better check here is to test if there are modules rather than if there are files since empty files have no parse.
@@ -124,6 +116,73 @@ func TestTemplateContentsForFile(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestTemplateContentsForFileInWorkspaceRoot(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Root is not allowed and is an error which can be silently ignored if the caller understands it.
} | ||
} | ||
|
||
func TestTemplateContentsForFileWithUnknownRoot(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the root is unknown, but there is a path, we can attempt to use the server's root URI as a root instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with one comment consider. Even if we use an error for "control flow", I don't think we'll want to log this as such
@@ -989,7 +989,9 @@ func (l *LanguageServer) StartTemplateWorker(ctx context.Context) { | |||
// determine the new contents for the file, if permitted | |||
newContents, err := l.templateContentsForFile(job.URI) | |||
if err != nil { | |||
l.logError(fmt.Errorf("failed to template new file: %w", err)) | |||
if !errors.Is(err, &templatingInRootError{}) { | |||
l.logError(fmt.Errorf("failed to template new file: %w", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really an error unless it's something the user asked for and didn't get? If I saw this in the logs, I would probably think I did something wrong and that I should look into how to fix it.
|
||
err = os.WriteFile(filepath.Join(td, ".regal/config.yaml"), []byte{}, 0o600) | ||
if err != nil { | ||
t.Fatalf("failed to create directory %s: %s", filepath.Join(td, ".regal"), err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not creating a directory
This also disables templating for files in the root.
Fixes #1164
Related to #1141, but this needs more work
Sorry for the low res video...
Screen.Recording.2024-10-03.at.18.16.04.mov