🧹 Use element plugin registry to define LibraryType #1427
Conversation
s-weigand
requested review from
jsnel,
a team and
joernweissenborn
as code owners
|
|
![sourcery-ai[bot]](/~https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Refactoring
PR Summary: The pull request focuses on refining the definition of LibraryType by utilizing the element plugin registry, ensuring that only explicitly added elements are considered valid types. This change aims to exclude non-instantiable types like ExtendableElement from being part of LibraryType. Additionally, the PR includes updates to the pre-commit configuration and the removal of an internal mock element used for testing.
Decision: Comment
📝 Type: 'Refactoring' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
General suggestions:
- Consider adding comments next to the type:ignore directives to explain why type checking is being bypassed, which will help future maintainers understand the context.
- Ensure that the removal of the InternalMockElement and the changes to LibraryType do not have unintended side effects on the existing tests and other parts of the codebase.
- Verify that all code interacting with the LibraryType is compatible with the new type structure introduced by the use of the plugin registry.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
|
|
||
| LibraryType: TypeAlias = dict[str, Element.get_annotated_type()] # type:ignore[misc,valid-type] | ||
| LibraryType: TypeAlias = dict[ # type:ignore[misc,valid-type] |
There was a problem hiding this comment.
suggestion (llm): The use of type:ignore may be necessary here, but it's generally a good practice to add a comment explaining why type checking is being ignored to aid future maintainers.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #1427 +/- ##
=========================================
- Coverage 84.9% 84.9% -0.1%
=========================================
Files 91 91
Lines 3744 3742 -2
Branches 728 728
=========================================
- Hits 3180 3178 -2
Misses 450 450
Partials 114 114 ☔ View full report in Codecov by Sentry. |
* 🧹 Use element plugin registry to define LibraryType * 🧹 Remove InternalMockElement * 🧰⬆️ Update pre-commit config
By using the plugin registry instead of looking up inheritance we can ensure that only elements that were explicitly added are valid types (e.g.
ExtendableElementis not instantiable by itself and should not be part ofLibraryType).Change summary
Checklist