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

Continue parsing module files on error #1172

Merged
merged 2 commits into from
Feb 13, 2023
Merged

Conversation

dbanck
Copy link
Member

@dbanck dbanck commented Feb 10, 2023

This PR changes the parsing of module files so that if we encounter an inaccessible file, we continue parsing all the other module files instead of aborting with an error.

That should improve the experience for emacs users, relying on magic symlinks to keep track of open files.

Fixes #1067

UX

Before
CleanShot 2023-02-13 at 11 55 20@2x
No features available within the module

After
CleanShot 2023-02-13 at 11 56 03@2x

@dbanck dbanck added the bug Something isn't working label Feb 10, 2023
return nil, nil, err
// If a file isn't accessible, continue with reading the
// remaining module files
continue
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense for the edge case described in #1067 but have we considered what impact this would have on real *.tf files which aren't accessible, e.g. due to mis-set permissions?

I'm guessing that we are just hoping that both client and server would be aligned on the "readability" of the same file, i.e. they'd be launched under the same user and have the same level of access. i.e. if we can't index the file, it also won't come through textDocument/didOpen, right?

So probably nothing to do here - just thinking out loud about the consequences and making sure we're aligned.

One edge case which comes to mind is - what if the user changes permissions at runtime - I'm guessing that this would come through the watch event, right? And presumably it comes before didOpen event, so there should be no race condition.

Copy link
Member Author

@dbanck dbanck Feb 13, 2023

Choose a reason for hiding this comment

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

That's a valid concern, thanks for raising it. I did some testing with mis-set permissions, but I can't create a test case. It seems impossible commit such a file, because git can't access it 😅

CleanShot 2023-02-13 at 12 02 10@2x

When working with an inaccessible file, the language server will skip it, and VS Code can't open the file, so it shouldn't come through textDocument/didOpen.
CleanShot 2023-02-13 at 11 56 10@2x

If the file permissions change, the watcher will pick it up and the LS will rerun all module related jobs:
CleanShot 2023-02-13 at 12 06 12@2x

@dbanck dbanck requested a review from a team as a code owner February 13, 2023 11:01
@dbanck dbanck merged commit b214b93 into main Feb 13, 2023
@dbanck dbanck deleted the b-handle-invalid-symlinks branch February 13, 2023 11:20
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mishandling of hidden files & emacs backup files
3 participants