Skip to content

Conversation

@fviernau
Copy link
Member

@fviernau fviernau commented Nov 14, 2025

Clearly separate again the upstream data model, from means to mapping it to ORT's model.
Furthermore, eliminate complexities related to the use of inner classes and mapping a Lockfile to another Lockfile, e.g in withResolvedPaths().
There was also a hidden requirement to not access the (now removed) lazy properties too early.

@fviernau fviernau requested a review from a team as a code owner November 14, 2025 10:30
@fviernau fviernau force-pushed the cococapods-lockfile-model-improvements branch from b473ca4 to 2e2e47a Compare November 14, 2025 10:43
@codecov
Copy link

codecov bot commented Nov 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.38%. Comparing base (10b1231) to head (b456617).

Additional details and impacted files
@@            Coverage Diff            @@
##               main   #11085   +/-   ##
=========================================
  Coverage     57.38%   57.38%           
+ Complexity     1703     1701    -2     
=========================================
  Files           346      346           
  Lines         12833    12833           
  Branches       1218     1218           
=========================================
  Hits           7364     7364           
  Misses         4999     4999           
  Partials        470      470           
Flag Coverage Δ
funTest-external-tools 13.47% <ø> (ø)
funTest-no-external-tools 31.06% <ø> (-0.11%) ⬇️
test-ubuntu-24.04 42.42% <ø> (ø)
test-windows-2025 42.40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fviernau fviernau force-pushed the cococapods-lockfile-model-improvements branch 4 times, most recently from 50191db to 27ea0af Compare November 14, 2025 11:42
Copy link
Member

@sschuberth sschuberth left a comment

Choose a reason for hiding this comment

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

For the record, I'm not a big fan of making DependencyHandler implementations stateful. I like APIs that ensure correct use at compile-time, but with the stateful design you have to remember to call setContext() and will only realize at runtime that something is wrong.

I know that the Yarn implementations already are stateful, too, and that we have no clear rule in favor or against this. I just wanted to share my opinion.

I also think that @bs-ondem should share his opinion as he contributed the withResolvedPaths() logic.

@sschuberth sschuberth requested a review from bs-ondem November 14, 2025 16:27
Simplify an upcoming change.

Signed-off-by: Frank Viernau <[email protected]>
Prepare for making `Dependency` a non-inner class again.

Signed-off-by: Frank Viernau <[email protected]>
Remove mixing the upstream data model with means for mapping it to ORT's
model. Also revert back to a non-inner data class, which removes
unnecessary complexity.

Signed-off-by: Frank Viernau <[email protected]>
Clearly separate the upstream data model, and simplify things by
turning the class into a non-inner data class again.

Signed-off-by: Frank Viernau <[email protected]>
Remove complications which have become unnecessary with recent
changes in `Lockfile`.

Signed-off-by: Frank Viernau <[email protected]>
Make the names a tad more speaking and consistent.

Signed-off-by: Frank Viernau <[email protected]>
@fviernau fviernau force-pushed the cococapods-lockfile-model-improvements branch from 27ea0af to b456617 Compare November 14, 2025 23:36
@fviernau fviernau requested a review from sschuberth November 14, 2025 23:36
@sschuberth sschuberth enabled auto-merge (rebase) November 15, 2025 17:00
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.

3 participants