oras icon indicating copy to clipboard operation
oras copied to clipboard

refactor: simplify manifest index metadata handler

Open TerryHowe opened this issue 8 months ago • 4 comments

What this PR does / why we need it:

This is kind of an example of why composition can be better than inheritance.

TerryHowe avatar Mar 29 '25 15:03 TerryHowe

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.69%. Comparing base (a38c2df) to head (6e8250b). Report is 43 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1673      +/-   ##
==========================================
- Coverage   84.70%   84.69%   -0.02%     
==========================================
  Files         126      126              
  Lines        5682     5678       -4     
==========================================
- Hits         4813     4809       -4     
  Misses        619      619              
  Partials      250      250              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Mar 29 '25 15:03 codecov[bot]

Hi @TerryHowe, thanks for your contribution.

Having separate OnIndexCreated and Render looks unnecessary, but this is our design to ensure future extensibility. Let me explain.

First of all, we need metadata.Render method for almost all commands' metadata handler, to signal that no more operations are coming and metadata can be printed. It has to be implemented by discard, and as go does not support method overloading, the Render of every command has to have the same parameters. As a result, we made it an interface and almost all metadata handlers implement it.

But we do need to pass in data for some commands, that's why we have functions like OnIndexCreated and OnAttached. In this way, when we add a command in the future, we can implement the metadata handler and discard neatly.

So we are not going to proceed with this PR, but thanks for your work anyway.

wangxiaoxuan273 avatar Apr 07 '25 09:04 wangxiaoxuan273

I think there are a couple more that do not need the Render method. There is no need to rigidly follow the same pattern for all metadata handlers when it adds no value. It adds complexity and confusion.

TerryHowe avatar Apr 13 '25 21:04 TerryHowe

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar May 29 '25 02:05 github-actions[bot]