[cli] format `find` output #794

Saviq · 2019-05-20T07:15:20Z

No description provided.

codecov · 2019-05-20T07:46:57Z

Codecov Report

Merging #794 into master will increase coverage by 0.57%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #794      +/-   ##
==========================================
+ Coverage   67.69%   68.26%   +0.57%     
==========================================
  Files         177      177              
  Lines        6219     6268      +49     
==========================================
+ Hits         4210     4279      +69     
+ Misses       2009     1989      -20

Impacted Files	Coverage Δ
include/multipass/cli/json_formatter.h	`100% <ø> (ø)`	⬆️
include/multipass/cli/yaml_formatter.h	`100% <ø> (ø)`	⬆️
src/client/cli/cmd/find.h	`100% <ø> (ø)`	⬆️
include/multipass/cli/csv_formatter.h	`100% <ø> (ø)`	⬆️
include/multipass/cli/table_formatter.h	`100% <ø> (ø)`	⬆️
include/multipass/cli/formatter.h	`50% <ø> (ø)`	⬆️
src/client/cli/formatter/format_utils.cpp	`89.13% <100%> (+2.64%)`	⬆️
src/client/cli/formatter/yaml_formatter.cpp	`98.85% <100%> (+0.25%)`	⬆️
src/client/cli/formatter/json_formatter.cpp	`100% <100%> (ø)`	⬆️
src/client/cli/formatter/table_formatter.cpp	`93.67% <100%> (+1.24%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd31507...a21c29c. Read the comment docs.

codecov · 2019-05-20T07:46:58Z

Codecov Report

Merging #794 into master will increase coverage by 0.59%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #794      +/-   ##
==========================================
+ Coverage   67.69%   68.28%   +0.59%     
==========================================
  Files         177      177              
  Lines        6219     6272      +53     
==========================================
+ Hits         4210     4283      +73     
+ Misses       2009     1989      -20

Impacted Files	Coverage Δ
include/multipass/cli/json_formatter.h	`100% <ø> (ø)`	⬆️
include/multipass/cli/yaml_formatter.h	`100% <ø> (ø)`	⬆️
src/client/cli/cmd/find.h	`100% <ø> (ø)`	⬆️
include/multipass/cli/csv_formatter.h	`100% <ø> (ø)`	⬆️
include/multipass/cli/table_formatter.h	`100% <ø> (ø)`	⬆️
include/multipass/cli/formatter.h	`50% <ø> (ø)`	⬆️
src/client/cli/formatter/format_utils.cpp	`89.58% <100%> (+3.09%)`	⬆️
src/client/cli/formatter/yaml_formatter.cpp	`98.86% <100%> (+0.27%)`	⬆️
src/client/cli/formatter/json_formatter.cpp	`100% <100%> (ø)`	⬆️
src/client/cli/formatter/table_formatter.cpp	`93.58% <100%> (+1.16%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd31507...9c72fb2. Read the comment docs.

ricab

Elegant, clear, precise. Quality coding here! I'm ready to approve, I'll just wait for your reply on a couple of comments.

src/client/cli/cmd/find.cpp

src/client/cli/formatter/table_formatter.cpp

gerboland

The --show-unsupported table and CSV output has this for me:

core,snapcraft,core16,,Snapcraft builder for Core 16,20190515
core18,snapcraft,,,Snapcraft builder for Core 18,20190515
core,,core16,Ubuntu,Core 16,20190424
core18,,,Ubuntu,Core 18,20190213

but the YAML & JSON do not include the duplicated entries "core" and "core18" - they only have the last ones.

Very edge-case, I presume this just points out the problem with duplicate image names?

Other than issue @ricab pointed out, it's fine

Saviq

Very edge-case, I presume this just points out the problem with duplicate image names?

Right, I'll need to include the remote name in there after all.

src/client/cli/formatter/table_formatter.cpp

Saviq · 2019-05-20T13:49:09Z

Right, I'll need to include the remote name in there after all.

@gerboland fixed, there's some duplication with the image_id thus, think it worth it abstracting out into format_utils.cpp? At first I thought I'd use a fmtlib formatter, but I'd have to modify the _ImageInfo instance to filter the aliases out…

gerboland · 2019-05-20T14:08:02Z

tests/test_output_formatter.cpp

+                           "            \"version\": \"20190520\"\n"
+                           "        }\n"
+                           "    }\n"
+                           "}\n";


Just a suggestion, raw string literals might make this nicer on the eye: https://stackoverflow.com/a/5460235

gerboland · 2019-05-20T14:13:31Z

@gerboland fixed, there's some duplication with the image_id thus, think it worth it abstracting out into format_utils.cpp? At first I thought I'd use a fmtlib formatter, but I'd have to modify the _ImageInfo instance to filter the aliases out…

I don't think it's worth the trouble tbh.

ricab · 2019-05-20T14:27:36Z

I don't think it's worth the trouble tbh.

@Saviq @gerboland I beg to differ. The "trouble" is negligible and quite worth eliminating triplication. This is the sort of thing that accumulates with time and makes bigger trouble in the long run. Just my opinion.

Saviq · 2019-05-20T14:42:24Z

@ricab @gerboland abstracted out a tiny utility for this then.

ricab · 2019-05-20T14:50:41Z

@Saviq perfect, happy codebase says thanks 😃 And me too, thanks a lot!

ricab

Detail: autocomplete for --format. Can also just be offloaded to a separate issue ~~(it's missing for --show-unsupported too)~~ or just left alone if we consider it an advanced option we don't want to complete. @Saviq I'll wait to know what you want to do and then approve.

ricab

OK approving then

bors r=gerboland,ricab

794: [cli] format `find` output r=gerboland,ricab a=Saviq Co-authored-by: Michał Sawicz <[email protected]>

bors · 2019-05-20T15:49:21Z

Build succeeded

794: [cli] format `find` output r=gerboland,ricab a=Saviq Co-authored-by: Michał Sawicz <[email protected]>

Saviq added 2 commits May 20, 2019 09:13

[list] rename "Release" to "Image" (Fixes #603)

07b8e9a

[cli] add filter_aliases in format_utils

402ad12

Saviq added 5 commits May 20, 2019 10:13

[cli] add table formatter for FindReply

d9becfd

[cli] add JSON formatter for FindReply

8a43dde

[cli] add CSV formatter for FindReply

8fed826

[cli] add YAML formatter for FindReply

998de54

[cli] add --format option to find command (Fixes #765, #530)

1233271

Saviq force-pushed the format-find-output branch from a21c29c to 1233271 Compare May 20, 2019 08:14

ricab reviewed May 20, 2019

View reviewed changes

src/client/cli/cmd/find.cpp Outdated Show resolved Hide resolved

src/client/cli/formatter/table_formatter.cpp Outdated Show resolved Hide resolved

gerboland reviewed May 20, 2019

View reviewed changes

Saviq commented May 20, 2019

View reviewed changes

src/client/cli/formatter/table_formatter.cpp Outdated Show resolved Hide resolved

Saviq force-pushed the format-find-output branch from 4b90041 to 174f3e3 Compare May 20, 2019 13:10

[cli] fix no-os case in table formatter output for FindReply

64c85bf

Saviq force-pushed the format-find-output branch from 174f3e3 to 64c85bf Compare May 20, 2019 13:13

[cli] include remote name in all formatters' output

a913271

[cli] drop unused includes

4bbf66c

gerboland reviewed May 20, 2019

View reviewed changes

[cli] factor out a image_string_for(alias) utility

5b52135

ricab reviewed May 20, 2019

View reviewed changes

[cli] add --format bash completion

9c72fb2

ricab approved these changes May 20, 2019

View reviewed changes

bors bot added a commit that referenced this pull request May 20, 2019

Merge #794

7e4c7d1

794: [cli] format `find` output r=gerboland,ricab a=Saviq Co-authored-by: Michał Sawicz <[email protected]>

bors bot merged commit 9c72fb2 into master May 20, 2019

bors bot deleted the format-find-output branch May 20, 2019 15:49

Saviq added a commit that referenced this pull request May 28, 2019

Merge #794

50ef988

794: [cli] format `find` output r=gerboland,ricab a=Saviq Co-authored-by: Michał Sawicz <[email protected]>

Saviq added a commit that referenced this pull request May 28, 2019

Merge #794

c9984ea

794: [cli] format `find` output r=gerboland,ricab a=Saviq Co-authored-by: Michał Sawicz <[email protected]>

Saviq added a commit that referenced this pull request May 28, 2019

Merge #794

e9a9fbb

794: [cli] format `find` output r=gerboland,ricab a=Saviq Co-authored-by: Michał Sawicz <[email protected]>

[cli] format find output #794

[cli] format find output #794

Uh oh!

Conversation

Saviq commented May 20, 2019

Uh oh!

codecov bot commented May 20, 2019

Codecov Report

Uh oh!

codecov bot commented May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gerboland left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Saviq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Saviq commented May 20, 2019

Uh oh!

gerboland May 20, 2019

Choose a reason for hiding this comment

Uh oh!

gerboland commented May 20, 2019

Uh oh!

ricab commented May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Saviq commented May 20, 2019

Uh oh!

ricab commented May 20, 2019

Uh oh!

ricab left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented May 20, 2019

Build succeeded

Uh oh!

Uh oh!

[cli] format `find` output #794

[cli] format `find` output #794

codecov bot commented May 20, 2019 •

edited

Loading

gerboland left a comment •

edited

Loading

ricab commented May 20, 2019 •

edited

Loading

ricab left a comment •

edited

Loading