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

fix crash on checking excluded folders with missing project data #2276

Merged
merged 2 commits into from
May 23, 2023

Conversation

rchl
Copy link
Member

@rchl rchl commented May 20, 2023

Ensure that the _folders_exclude_patterns list has same number of elements as folders so that accessing by index doesn't throw when window.project_data() is missing.

Fixes TerminalFi/LSP-copilot#109

@@ -63,7 63,7 @@ def __init__(self, window: sublime.Window) -> None:
self._update_exclude_patterns(self.folders)

def _update_exclude_patterns(self, folders: List[str]) -> None:
self._folders_exclude_patterns = []
self._folders_exclude_patterns = [[]] * len(folders)
project_data = self._window.project_data()
Copy link
Member

Choose a reason for hiding this comment

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

I would definitely put a comment on that line, or link to the issue,
it was not obvious why it needs to be multiplied by the length of the folders

Copy link
Member

@jwortmann jwortmann May 22, 2023

Choose a reason for hiding this comment

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

[x] * n is a concise syntax in Python to create a list with n entries which are all x, i.e. here it will be for example [[], [], []] with 3 folders.
But yeah, I was also surprised that this bug can happen, and I never saw it with "normal" usage. The window.project_data() even returns a dict with the (sidebar) folders, if there is no project at all (iirc ST automatically creates a "hidden" project in that case), or when ST is started by double-clicking a single .sublime-workspace file of a project. It might be possible with multiple workspace files or if you edit them manually.
But with this initialization line we are certain to be on the safe side, so it looks fine to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that those lists created via [[]] * len(folders) refer to exactly the same thing. Not sure whether this is what you want but just provide a potential issue.

>>> a = [[]] * 4
>>> a[0].append('foo')
>>> a
[['foo'], ['foo'], ['foo'], ['foo']]

Copy link
Member

Choose a reason for hiding this comment

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

It should be fine, because each entry gets replaced completely, not appended to:

self._folders_exclude_patterns[i] = exclude_patterns

For example

>>> a = [[]] * 4
>>> a[1] = ['foo']
>>> a
[[], ['foo'], [], []]

Copy link
Member Author

@rchl rchl May 22, 2023

Choose a reason for hiding this comment

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

btw. it's expected that this creates a list of lists since exclude patterns are a list and there would be one of those for each folder.
The current code would also be fine with other value (like None) but this is more consistent with the real (correct) case.

@rchl rchl merged commit 9ec8001 into main May 23, 2023
@rchl rchl deleted the fix/exclude-folders branch May 23, 2023 06:39
rchl added a commit that referenced this pull request May 30, 2023
* main:
  fix "Error rewriting command" warning triggered on startup (#2277)
  Take font style of sighelp active parameter from color scheme (#2279)
  Add argument "include_declaration" to "lsp_symbol_references" (#2275)
  fix crash on checking excluded folders with missing project data (#2276)
  Fix tagged diagnostics flickering on document changes (#2274)
  Cut 1.24.0
  use class for diagnostic info instead of hardcoding color (#2257)
  Fix package storage path in a docstring description (#2256)
  Use regular font style in sighelp popup if already highlighted by color scheme (#2259)
  update note about custom color scheme rule used for diagnostics (#2254)
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.

Incompatible with any sublime-workspace?
5 participants