-
Notifications
You must be signed in to change notification settings - Fork 185
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
Conversation
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 |
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 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:
(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 ;)). |
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.
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... ;)
ProjectFolders now holds a mapping of
folder_exclude_patterns
matching each of the workspacefolders
. 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