CLI Fixes / NPM dependency updates #35
Conversation
* `_parse_type_annotation`: Enhanced validation for argument metadata. * `parse_action`: Improved checks for decorators and their arguments. * used `pop` for key removal in the `connect` cli command * updated NPM dependencies for website
|
@cvs0 is attempting to deploy a commit to the Admyral Team on Vercel. A member of the Team first needs to authorize it. |
|
|
| action_decorator = decorator | ||
| break |
There was a problem hiding this comment.
I think we should actually raise an error here because display_name and display_namespace must always be defined.
| if action_decorator is None: | ||
| raise ValueError("Action function must have the @action decorator.") | ||
|
|
||
| if isinstance(action_decorator, ast.Call): |
There was a problem hiding this comment.
Can become an assert if we throw in the case of isinstance(decorator, ast.Name).
| if isinstance(action_decorator, ast.Call): | ||
| # Ensure the decorator has arguments if expected | ||
| if not action_decorator.keywords: | ||
| raise ValueError("The @action decorator must include arguments.") |
There was a problem hiding this comment.
| raise ValueError("The @action decorator must include arguments.") | |
| raise ValueError("The arguments display_name and display_namespace must be defined for the @action decorator.") |
| for arg in action_decorator.keywords: | ||
| if not isinstance(arg.value, (ast.Constant, ast.List)): | ||
| raise ValueError(f"Invalid argument type in @action decorator: {arg.arg} must be a constant or list.") |
There was a problem hiding this comment.
Good idea to validate the values. However, I would suggest a different approach due to maintainability and granularity reasons. It would probably make sense to add a parameter value validation function in admyral/action.py which is then used before a new action is constructed in the decorator and also here. If we maintain this function in action.py and a new parameter is added then it is harder to miss updating the validation function.
| if not arg_metadata_params: | ||
| raise ValueError("ArgumentMetadata must contain at least one parameter.") |
There was a problem hiding this comment.
This check is not needed imo because this is already handled by ArgumentMetadata.model_validate
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Co-authored-by: Daniel Grittner <danielgrittner@web.de>
Co-authored-by: Daniel Grittner <danielgrittner@web.de>
|
@cvs0 Just checking in: do you plan to work on the requested changes any time soon? otherwise I'll take this over |
CLI Fixes / NPM Dependency Updates
Summary of items Affected
_parse_type_annotation: Enhanced validation for argument metadata.parse_action: Improved checks for decorators and their arguments.connectcli command, improved config reading.Type of change
Mandatory Tasks
Checklist