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 handling of folder_exclude_patterns in projects #2237

Merged
merged 4 commits into from
Apr 22, 2023
Merged

Fix handling of folder_exclude_patterns in projects #2237

merged 4 commits into from
Apr 22, 2023

Conversation

rchl
Copy link
Member

@rchl rchl commented Apr 17, 2023

ProjectFolders now holds a mapping of folder_exclude_patterns matching each of the workspace folders. This way we can do more targeted check to know whether path is not ignored in at least one of the folders.

I think this logic might be a bit hard to wrap the head around and not sure if the design of that fix is good.

Fixes #2171

@rchl rchl requested a review from jwortmann April 17, 2023 19:43
@jwortmann
Copy link
Member

jwortmann commented Apr 19, 2023

Could you explain in a nutshell how does this fix the linked issue? From a quick look, it seems to me that it does the same as before, just with additional caching instead of calling the API functions each time. Because in the includes_excluded_path function, the loop still breaks when a pattern matches and then returns True. The bug report is about the situation when a pattern matches for a particular folder, but in the project data this same folder (or subfolder) is listed another time again later, so that we actually would need to ignore this match. (I might be wrong because I haven't tested)

@rchl
Copy link
Member Author

rchl commented Apr 19, 2023

Could you explain in a nutshell how does this fix the linked issue? From a quick look, it seems to me that it does the same as before, just with additional caching instead of calling the API functions each time. Because in the includes_excluded_path function, the loop still breaks when a pattern matches and then returns True. The bug report is about the situation when a pattern matches for a particular folder, but in the project data this same folder (or subfolder) is listed another time again later, so that we actually would need to ignore this match. (I might be wrong because I haven't tested)

There would ideally be tests for this and those should make it clearer what the intentions are... I think it would be easy to make low-level unit tests but I would prefer higher level end to end tests but not sure if it's possible to mock window.project_data().

That said, you are right that this logic was incorrect. I did accidentally reverse the condition when doing last minute refactorings. Should be better now.

But in a nutshell:

  • if the file is not contained within project folders (because there are no folders or because it's not part of existing folders) and hide_non_project_diagnostics is not set then we don't ignore diagnostics
  • if there are project folders and the file is within one or more of those folders then it must not be excluded in at least one of those folders for the diagnostics to not be ignored.

(I wish I would find a better way to explain it as it doesn't sound very clear even to me, when I'm writhing it ;)).

Copy link
Member

@jwortmann jwortmann left a comment

Choose a reason for hiding this comment

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

Okay, I think the logic is correct now.

At a later point, we could try to convert this function into an is_visible_in_sidebar, by also honoring file_exclude_patterns, file_include_patterns and folder_include_patterns from the project settings. But all this path handling and conditional logic is quite error prone and no fun to implement, so I will probably skip on that... ;)

@rchl rchl merged commit 2a1d4f3 into main Apr 22, 2023
@rchl rchl deleted the fix/excludes branch April 22, 2023 21:15
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.

Missing diagnostics due to "folder_exclude_patterns" with multiple project folders
2 participants