feat(medcat-trainer): trainer-ee-scaffolding#524
Conversation
mart-r
left a comment
mart-r
left a comment
There was a problem hiding this comment.
Overall looks good to me!
A few comments on type informations in extensions.
A question / suggestion regarding tyhe typing of permission hooks.
A note about shallow copies in extensions/plugins routes management.
A suggestion to simplify entry point discovery (no need to support python<3.9-style).
PS:
Didn't look at the frontend stuff really.
| pre_document_submit = Signal() | ||
|
|
||
| #: Sent immediately after a document is submitted to training. | ||
| #: kwargs: ``project``, ``document``, ``user``. |
There was a problem hiding this comment.
I guess type information is consistent with the one above, but do we perhaps want to be explicit about it?
| post_document_submit = Signal() | ||
|
|
||
| #: Sent after an :class:`AnnotatedEntity` row is created. | ||
| #: kwargs: ``annotation`` (AnnotatedEntity), ``project``, ``document``, |
There was a problem hiding this comment.
Here as well, type information for project and document isn't included.
| annotation_created = Signal() | ||
|
|
||
| #: Sent after an :class:`AnnotatedEntity` row is updated. | ||
| #: kwargs: ``annotation``, ``project``, ``document``, ``user``. |
| annotation_updated = Signal() | ||
|
|
||
| #: Sent after an :class:`AnnotatedEntity` row is deleted. | ||
| #: kwargs: ``annotation`` (instance prior to delete), ``project``, ``document``. |
| annotation_deleted = Signal() | ||
|
|
||
| #: Sent after a :class:`ProjectGroup` row is created. | ||
| #: kwargs: ``project_group``. |
|
|
||
|
|
||
| def get_menu_extensions() -> list[dict[str, Any]]: | ||
| return [dict(it) for it in _menu_extensions] |
There was a problem hiding this comment.
Same note here regarding shallow copy
| raise TypeError("route must be a dict") | ||
| if "path" not in route or "component" not in route: | ||
| raise ValueError("route requires 'path' and 'component'") | ||
| _plugin_routes.append(dict(route)) |
There was a problem hiding this comment.
Same note here regarding shallow copy
|
|
||
|
|
||
| def get_routes() -> list[dict[str, Any]]: | ||
| return [dict(r) for r in _plugin_routes] |
There was a problem hiding this comment.
Same note here regarding shallow copy
| try: | ||
| try: | ||
| eps = entry_points(group='mct.plugins') | ||
| except TypeError: |
There was a problem hiding this comment.
We don't need this except clause. This would only be applicable for pre python 3.10:
https://docs.python.org/3/library/importlib.metadata.html#entry-points
3.9 (and below) are end of life, so I don't think we need to keep trying to support them.
Plus, we're running 3.12 for the image anyway.
| # used. Hooks MUST NOT be used to deny access that the OSS code would | ||
| # otherwise grant. | ||
|
|
||
| PermissionHook = Callable[..., Optional[bool]] |
There was a problem hiding this comment.
Theser have an unknown argument signature. Is there a reason not to force args User and ProjectAnnotateEntites? That's what's being passed in api.permissions.is_project_admin, right? Or is that just because the method it's used in doesn't have typing? Perhaps at the very least define as 2 arguments as [Any, Any]?
There was a problem hiding this comment.
good point - have typed and limited to the two args used
|
|
||
|
|
||
| def is_project_admin(user, project): | ||
| def is_project_admin(user: [User], project: [ProjectAnnotateEntities]): |
There was a problem hiding this comment.
I don't think this is correct type annotation?
2 participants
Scaffolding changes for the mct OSS project here according to the plan
Plugin scaffolding changes for
TrainAnnotationsProjectAdminand security features in mct-ee