One conversion path in LibraryService instead of six copies of it - #47
Open
lgnap wants to merge 1 commit into
Open
One conversion path in LibraryService instead of six copies of it#47lgnap wants to merge 1 commit into
lgnap wants to merge 1 commit into
Conversation
LibraryService held three public conversion methods, each branching on the source format, so the skeleton -- observe the source, make a temporary, read, transform assets, write, move into the library, record provenance -- was written out six times. They are one method now, parameterised by reader, asset transform, writer and labels. The copies had already drifted, which is the argument for doing this: - five logged "Pack is in <x> format", the archive-to-FS one logged "Pack to transfer is in <x> format"; - three took the destination UUID from the pack before the asset transform and three from the pack after. Neither difference changed behaviour. The transforms in PackAssetsCompression mutate and return the same instance, so the two UUIDs are the same object, and no test asserts log text. But keeping six copies of an invariant in step by hand is the thing that eventually does change behaviour, and the invariant here is not decorative: the source is observed before anything reads it and again afterwards, the provenance record is written only if the two agree, and the output is moved into the library only once the conversion has succeeded. The one visible change is that log line, now consistent with the other five. No test was added or modified. That is the point: ConversionProvenanceTest, ConversionStreamLifecycleTest and ConversionVerificationTest are the safety net, and a refactor that needed them edited would have changed behaviour. They pass untouched -- 11, 4 and 13 cases -- and the suite is 284 tests, 0 failures, 39 skipped on Linux, identical to the commit this sits on. 870 lines to 806.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the duplication in
LibraryServicewe discussed on my fork's #4, which you said was open on your side.What it does
Three public conversion methods, each branching on the source format, meant the skeleton was written out six times — observe the source, make a temporary, read, transform assets, write, move into the library, record provenance. It is one private method now, parameterised by a reader, an asset transform, a writer and two labels.
870 lines to 806.
Why it is worth doing, from the copies themselves
The six had already drifted apart, in two ways I found while reading them rather than by looking for them:
Pack is in <x> format. The archive-to-FS one logsPack to transfer is in <x> format.Neither changed behaviour. The three
PackAssetsCompression.with*methods mutate their argument andreturn pack, the same instance, so the two UUIDs are the same object — worth knowing given thewith...naming reads as if they copy. And nothing asserts log text.So this is not a bug report. It is that the invariant those six copies encode is not decorative — the source is observed before anything reads it and again after, the provenance record is written only if the two agree, and the output is moved into the library only once the conversion succeeded — and holding that in step across six copies by hand is what eventually does change behaviour. It holds in one place now.
The one visible change is that log line, made consistent with the other five. Say the word if you would rather keep it as it was.
The check that matters
No test was added or modified. That is deliberate and it is the whole verification:
ConversionProvenanceTest,ConversionStreamLifecycleTestandConversionVerificationTestare the safety net you built for exactly this, and a refactor that needed them edited would have been a refactor that changed behaviour.They pass untouched — 11, 4 and 13 cases, with the 3 Windows-only lifecycle cases skipping on Linux as usual.
git statusonweb-ui/src/testis empty.Full suite: 284 tests, 0 failures, 39 skipped on Linux, on Temurin 11 — identical to
59c336c, which this sits directly on.Windows unverified locally, as before; your CI covers it.
One thing I noticed and did not touch
addConvertedFsPackFile(String packPath, Boolean allowEnriched)never usesallowEnriched— not before this change and not after. The FS writer takes no such flag. I kept the signature because it is public API and callers pass it, but it looks like it should either be honoured or removed. Happy to do either in a separate PR; it seemed wrong to fold an API change into a refactor whose entire claim is that nothing changed.