Skip to content
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

[#978][Improvement] Provides a tool class to format CLI output content #979

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

yl09099
Copy link
Contributor

@yl09099 yl09099 commented Jun 27, 2023

What changes were proposed in this pull request?

Provides a tool class to format CLI output content.

Why are the changes needed?

Fix: #978

Does this PR introduce any user-facing change?

The CLI command display is more beautiful.Similar to:

image

How was this patch tested?

Added UT.

@yl09099 yl09099 force-pushed the uniffle-978 branch 2 times, most recently from 4ba0128 to be1e07b Compare June 27, 2023 13:55
@jerqi jerqi requested a review from zuston June 27, 2023 15:05
@Test
public void testTableFormat() {

String expectStr = "+----------------------------------------"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way? This looks difficult to read. @zuston Do you have a good suggestion?

Copy link
Collaborator

@slfan1989 slfan1989 Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need 9 rows? @yl09099

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way? This looks difficult to read. @zuston Do you have a good suggestion?

Loading from the file?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks better, let's wait for @zuston the suggestion.

@yl09099 yl09099 force-pushed the uniffle-978 branch 2 times, most recently from b8ffd25 to 6c4becf Compare June 28, 2023 02:23
@codecov-commenter
Copy link

Codecov Report

Merging #979 (b8ffd25) into master (bb61efb) will increase coverage by 12.76%.
The diff coverage is 86.02%.

❗ Current head b8ffd25 differs from pull request most recent head 6c4becf. Consider uploading reports for the commit 6c4becf to get more accurate results

@@              Coverage Diff              @@
##             master     #979       +/-   ##
=============================================
+ Coverage     44.23%   57.00%   +12.76%     
- Complexity        0     2084     +2084     
=============================================
  Files            20      309      +289     
  Lines          2360    12971    +10611     
  Branches          0     1200     +1200     
=============================================
+ Hits           1044     7394     +6350     
- Misses         1245     5159     +3914     
- Partials         71      418      +347     
Impacted Files Coverage Δ
...n/java/org/apache/uniffle/cli/CLIContentUtils.java 86.02% <86.02%> (ø)

... and 328 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yl09099
Copy link
Contributor Author

yl09099 commented Jun 28, 2023

@jerqi This error is not caused by me, take a hard look

@yl09099
Copy link
Contributor Author

yl09099 commented Jun 28, 2023

It has been modified, please review it @slfan1989 @zuston

@zuston
Copy link
Member

zuston commented Jun 29, 2023

It has been modified, please review it @slfan1989 @zuston

Could this apply one cli command in this PR? I'm not sure we will use this in the current implementation currently.

Anyway, this looks good to me.

Copy link
Member

@zuston zuston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I got this motivation after reviewing related PRs

@zuston zuston merged commit f622aca into apache:master Jun 29, 2023
27 checks passed
@zuston
Copy link
Member

zuston commented Jun 29, 2023

Merged.

Thanks @yl09099

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Provides a tool class to format CLI output content
4 participants