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

refactor: Successor getting with separation of concerns #1443

Merged
merged 5 commits into from
Aug 9, 2024

Conversation

TerryHowe
Copy link
Member

What this PR does / why we need it:

The PrintSuccessorStatus method was complicated with the function passed in and it didn't show the best separation of concerns. The print module ideally wouldn't know about a fetcher for example.

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 85.59322% with 17 lines in your changes missing coverage. Please review.

Project coverage is 83.43%. Comparing base (c31d120) to head (d9e22e8).

Files Patch % Lines
cmd/oras/internal/display/status/text.go 80.76% 2 Missing and 3 partials ⚠️
cmd/oras/root/cp.go 28.57% 3 Missing and 2 partials ⚠️
internal/testutils/fetcher.go 83.33% 3 Missing ⚠️
cmd/oras/internal/display/status/tty.go 90.00% 2 Missing ⚠️
cmd/oras/root/attach.go 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1443      +/-   ##
==========================================
+ Coverage   83.29%   83.43%   +0.13%     
==========================================
  Files         110      112       +2     
  Lines        3892     3936      +44     
==========================================
+ Hits         3242     3284      +42     
- Misses        426      428       +2     
  Partials      224      224              

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

Copy link
Member Author

@TerryHowe TerryHowe left a comment

Choose a reason for hiding this comment

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

/lgtm

internal/descriptor/descriptor.go Outdated Show resolved Hide resolved
internal/descriptor/descriptor.go Outdated Show resolved Hide resolved
cmd/oras/internal/display/status/utils.go Outdated Show resolved Hide resolved
cmd/oras/internal/display/status/text.go Outdated Show resolved Hide resolved
cmd/oras/root/cp.go Outdated Show resolved Hide resolved
@qweeah
Copy link
Contributor

qweeah commented Jul 17, 2024

@TerryHowe We have changed the behavior of Println. Print error will now be returned as terminal errors. Can you help review the change, thanks.

I also tracked to-do fixes in other places in #1446

Copy link
Member Author

@TerryHowe TerryHowe 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 cannot approve this, but looks good

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 with suggestion. This PR also requires improved code testing. I'd suggest adding some unit tests.

cmd/oras/internal/output/print.go Outdated Show resolved Hide resolved
@TerryHowe
Copy link
Member Author

I was kind of hoping to do this as a follow up, but to increase test coverage, I moved descriptor.Successors to graph.FilteredSuccessors since it is just a specialization of graph.Successors and the framework to test it was in graph_test.go.

@TerryHowe
Copy link
Member Author

There is a bit of a chicken/egg situation here. The whole reason I went down this path of refactoring successors was to create a tty handler and once the tty handler was created, the code would be way more testable. I need to think about this.

Copy link
Contributor

@qweeah qweeah left a comment

Choose a reason for hiding this comment

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

LGTM, missing lines in tty.go and text.go can be covered by unit tests though.

@TerryHowe TerryHowe force-pushed the get-successors branch 6 times, most recently from 4c50b9c to 5d5d6ee Compare July 26, 2024 21:59
@TerryHowe TerryHowe force-pushed the get-successors branch 2 times, most recently from 54f841f to 12c4d07 Compare August 3, 2024 14:33
@qweeah
Copy link
Contributor

qweeah commented Aug 6, 2024

@TerryHowe Can you help add unit test to cover missing lines in cmd/oras/internal/display/status/text.go? Thanks

@qweeah
Copy link
Contributor

qweeah commented Aug 9, 2024

@TerryHowe You have chosen the hard way but you have done it good! Thanks for the contribution. Added several test to cover more lines. PTAL.

@TerryHowe
Copy link
Member Author

@TerryHowe You have chosen the hard way but you have done it good! Thanks for the contribution. Added several test to cover more lines. PTAL.

Thanks for working with me on this and all the others!

LGTM

@TerryHowe TerryHowe merged commit 141c5e1 into oras-project:main Aug 9, 2024
8 checks passed
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.

None yet

3 participants