-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Added result commands #30
base: main
Are you sure you want to change the base?
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
0311513
to
42e76b0
Compare
42e76b0
to
a550aed
Compare
|
||
|
||
@results.command(name="list") | ||
@click.argument("session-id", required=True, type=str) |
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.
Remove this argument. The user is free to list results as needed. The filters are here for that!
This change must also be applied on task list command.
"-f", | ||
"--filter", | ||
"filter_with", | ||
type=FilterParam("Partition"), |
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.
Small typo that should be “Result”.
src/armonik_cli/commands/results.py
Outdated
metavar="FILTER EXPR", | ||
) | ||
@click.option( | ||
"--sort-by", type=FieldParam("Task"), required=False, help="Attribute of task to sort with." |
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.
Idem typo!
results_list = [] | ||
while True: | ||
total, results = results_client.list_results( | ||
result_filter=(Result.session_id == session_id) & filter_with |
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.
here just put the filter and ignore the session_id!
"-r", | ||
"--result", | ||
"results", | ||
type=ResultParam(), |
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.
Use a more explicit name like ResultNameDataParam.
raise click.FileError( | ||
str(result_data_filepath), | ||
f"File {str(result_data_filepath)} not found for result {res[0]}", | ||
) |
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.
This should be done in the ResultParam.
@click.option( | ||
"-r", | ||
"--result", | ||
"results", |
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.
result_definitions
) | ||
|
||
# Enforce that at least one mutual option is provided | ||
if self.require_one and not (mutex or self.name in opts): |
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.
if self.require_one and not mutex and self.name not in opts
# Enforce that at least one mutual option is provided | ||
if self.require_one and not (mutex or self.name in opts): | ||
raise click.UsageError( | ||
f"At least one of the following options must be provided: {', '.join(self.mutual | {self.name})}." |
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.
.join(self.mutual)
@@ -188,3 +189,67 @@ def convert( | |||
) | |||
cls = getattr(common, self.base_struct) | |||
return getattr(cls, value) | |||
|
|||
|
|||
class ResultParam(click.ParamType): |
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.
Rework the implementation using namedtuples. The conversion should return a list of namedtuples with values for name and data.
Motivation
The current version of the CLI doesn't have result-specific commands.
Description
Added commands for :
Along with a parameter type for parsing result inputs.
Testing
Unit testing for different failing scenarios, serialization and commandline argument parsing.
Impact
Not applicable.
Additional Information
None.
Checklist