-
Notifications
You must be signed in to change notification settings - Fork 184
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
Conversation
@@ -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() |
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.
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
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.
[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.
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.
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']]
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.
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'], [], []]
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.
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.
* 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)
Ensure that the
_folders_exclude_patterns
list has same number of elements asfolders
so that accessing by index doesn't throw whenwindow.project_data()
is missing.Fixes TerminalFi/LSP-copilot#109