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.

Sign up for GitHub

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

Jump to bottom

refactor: list output handler #1176

Merged
merged 16 commits into from
Feb 26, 2025
Merged

refactor: list output handler #1176

merged 16 commits into from
Feb 26, 2025

Conversation

JeyJeyGao
Copy link
Contributor

@JeyJeyGao JeyJeyGao commented Feb 17, 2025 •
edited
Loading

Refactor:

  • moved the output logic of list command to ListHandler to simplify list command code logic
  • added streamPrinter to print tree node in streaming fashion

Test:

  • improved E2E

Resolve part of #1151

@JeyJeyGao
fix: complete codes
5820dbf 
Signed-off-by: Junjie Gao <[email protected]>
@codecov Codecov
Copy link

codecov bot commented Feb 17, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.70175% with 11 lines in your changes missing coverage. Please review.

Project coverage is 76.08%. Comparing base (ae0d432) to head (096b532).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...md/notation/internal/display/metadata/tree/list.go 75.00% 4 Missing and 2 partials ⚠️
...notation/internal/display/metadata/tree/printer.go 85.71% 2 Missing and 1 partial ⚠️
cmd/notation/list.go 80.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1176      +/-   ##
==========================================
- Coverage   76.11%   76.08%   -0.03%     
==========================================
  Files          69       71       +2     
  Lines        3596     3625      +29     
==========================================
+ Hits         2737     2758      +21     
- Misses        665      670       +5     
- Partials      194      197       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JeyJeyGao
test: improve E2E
21799b3 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: update interface
bc35b66 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: improve code
cde3cf1 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: update
e7bf3aa 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao JeyJeyGao marked this pull request as ready for review February 17, 2025 08:30
@JeyJeyGao
fix: improve E2E
3b32b0d 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: complete code
1772aa0 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao JeyJeyGao mentioned this pull request Feb 17, 2025
Open
5 tasks
@JeyJeyGao
fix: add streaming printer
b412fd9 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: update
de72925 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao JeyJeyGao requested a review from shizhMSFT February 18, 2025 04:38
@JeyJeyGao
fix: update
2abca21 
Signed-off-by: Junjie Gao <[email protected]>
@Two-Hearts Two-Hearts mentioned this pull request Feb 20, 2025
Closed
Two-Hearts
Two-Hearts previously approved these changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

LGTM

@JeyJeyGao
fix: update
83d602b 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: resolve comments
bbc19bb 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: update
3d75dc3 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: update
68550ac 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: update
3791fbf 
Signed-off-by: Junjie Gao <[email protected]>
shizhMSFT
shizhMSFT previously approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Two-Hearts
Two-Hearts previously approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

LGTM

@JeyJeyGao JeyJeyGao dismissed stale reviews from Two-Hearts and shizhMSFT via 096b532 February 26, 2025 02:52
Two-Hearts
Two-Hearts approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

LGTM

shizhMSFT
shizhMSFT approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@JeyJeyGao JeyJeyGao merged commit ba4c018 into notaryproject:main Feb 26, 2025
7 checks passed
@JeyJeyGao JeyJeyGao deleted the fix/list_display_handler branch February 26, 2025 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@Two-Hearts Two-Hearts Two-Hearts approved these changes

@gokarnm gokarnm Awaiting requested review from gokarnm gokarnm is a code owner

@NiazFK NiazFK Awaiting requested review from NiazFK NiazFK is a code owner

@priteshbandi priteshbandi Awaiting requested review from priteshbandi priteshbandi is a code owner

@rgnote rgnote Awaiting requested review from rgnote rgnote is a code owner

@toddysm toddysm Awaiting requested review from toddysm toddysm is a code owner

@vaninrao10 vaninrao10 Awaiting requested review from vaninrao10 vaninrao10 is a code owner

@yizha1 yizha1 Awaiting requested review from yizha1 yizha1 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JeyJeyGao @shizhMSFT @Two-Hearts