-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
added option to display resolution in ls command #86
Conversation
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.
Thanks for the PR 🎉
About the "not failing tests": most of the tests for the ls
command just check if a specific strings exists in the output, but not the exact output itself - thus, they pass.
It would still be great if you could add test cases for this new feature :)
Also, can you add a new changelog entry? You can use 86.feature
as a filename.
Thanks :)
@@ -59,27 +59,49 @@ def cli(): | |||
@click.option( | |||
"-j", "--json", "as_json", flag_value=True, help="Output result in JSON format" | |||
) | |||
def ls(types, as_json): # pylint: disable=invalid-name | |||
@click.option( | |||
"-p", |
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.
Why is it -p
?
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.
just copied what was used for the 'all' command
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.
let me know if you want me to remove it
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.
I see, my mistake - this was probably introduced when I renamed precision
to resolution
.
I'll change that in master :)
Please also rebase to the current master - I've added a fix to trigger Ci on Pull Requests :) |
Codecov Report
@@ Coverage Diff @@
## master #86 +/- ##
==========================================
+ Coverage 99.35% 99.36% +0.01%
==========================================
Files 8 8
Lines 308 313 +5
Branches 49 52 +3
==========================================
+ Hits 306 311 +5
Misses 2 2
Continue to review full report at Codecov.
|
check failure looks like a timeout in codecov |
Sorry, didn't add a new test case but existing cli tests pass.