-
Notifications
You must be signed in to change notification settings - Fork 2
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
Neat 239 a has data filter can reference at most 10 views and or containers #445
Neat 239 a has data filter can reference at most 10 views and or containers #445
Conversation
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.
Today, neat is never using a HasData filter on views, so I don't think we should introduce a Warning on too many views in a filter.
Furthermore, I would just raise the warning in the place were the filter is created. Then it is simpler, creating a PR to demonstrate. This seems more complex than necessary.
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
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.
One suggestion.
for view_id in view_inheritance: | ||
if implemented_view := indexed_views.get(view_id): | ||
inherited_referenced_containers |= implemented_view.referenced_containers() | ||
else: | ||
raise IncompleteSchemaError(missing_component=view_id).as_exception() | ||
|
||
return directly_referenced_containers | inherited_referenced_containers |
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.
Thanks this helped, I struggled with the last one.
indexed_views = { | ||
**{view.as_id(): view for view in self.views}, | ||
**({view.as_id(): view for view in self.reference.views} if self.reference else {}), | ||
} |
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.
Suggest adding this as a cached_property as it is independent of the method input.
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.
Nevermind, that will cause issues if more views are added later.
[0.76.3] - 10-05-24
Added