Improve assertion error message of SortingAnalyzer job kwargs #4178
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves error messages in the SortingAnalyzer.compute() method by adding detail about which invalid arguments were passed when too many arguments are provided.
Key Changes:
- Enhanced assertion error messages to include the list of unexpected parameter keys when users pass invalid arguments to
compute_several_extensions()
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| assert len(params_) == 0, ("Too many arguments for SortingAnalyzer.compute_several_extensions(), " | ||
| f"they are: {list(params_.keys())}") |
There was a problem hiding this comment.
The conversion list(params_.keys()) is redundant. Dictionary keys can be directly used in f-strings, which will display them in a more readable dict_keys format. Consider using f'they are: {params_.keys()}' or if a list format is preferred, f'they are: {set(params_)}' for cleaner output.
| params_, job_kwargs = split_job_kwargs(kwargs) | ||
| assert len(params_) == 0, "Too many arguments for SortingAnalyzer.compute_several_extensions()" | ||
| assert len(params_) == 0, ("Too many arguments for SortingAnalyzer.compute_several_extensions(), " | ||
| f"they are: {list(params_.keys())}") |
There was a problem hiding this comment.
The conversion list(params_.keys()) is redundant. Dictionary keys can be directly used in f-strings, which will display them in a more readable dict_keys format. Consider using f'they are: {params_.keys()}' or if a list format is preferred, f'they are: {set(params_)}' for cleaner output.
| f"they are: {list(params_.keys())}") | |
| f"they are: {set(params_)}") |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| assert len(params_) == 0, ( | ||
| "Too many arguments for SortingAnalyzer.compute_several_extensions(), " f"they are: {set(params_)}" | ||
| ) |
There was a problem hiding this comment.
The f-string prefix should be applied to the entire string, not just the second part. Move the 'f' prefix before the opening quote of the string or combine into a single f-string: f\"Too many arguments for SortingAnalyzer.compute_several_extensions(), they are: {set(params_)}\"
| assert len(params_) == 0, ( | ||
| "Too many arguments for SortingAnalyzer.compute_several_extensions(), " f"they are: {set(params_)}" | ||
| ) |
There was a problem hiding this comment.
The f-string prefix should be applied to the entire string, not just the second part. Move the 'f' prefix before the opening quote of the string or combine into a single f-string: f\"Too many arguments for SortingAnalyzer.compute_several_extensions(), they are: {set(params_)}\"
|
Copilot suggestions have been partially accepted, not accepting the second suggestion. Now the code LGTM. |
|
Hey @MGAMZ, it would be great if the error message told the user what to do to fix the error. Maybe instead of "they are: ..." we could say something like "please remove the arguments {set(params_)} from the compute function.". What'dya think? |
That would be better~ Commit done. |
Co-authored-by: Chris Halcrow <[email protected]>
|
@chrishalcrow My fault, missed another place. 😓 |
3 participants
When using
SortingAnalyzerandcompute, if user specifies too many args, there will be an assertion error. But the assertion message does not contain the detailed message about which arg violates the API.So I wish to improve the assertion message. The modification is simple and straightforward, should not introduce any logical breaking.