chore(refactor): rename SnapshotProducer::manifest_file to produce_manifest_file_list - #2596
Conversation
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
The rename reads well: produce_manifest_file_list accurately describes what the method does — it collects existing manifests, writes a new manifest when there are added data files, and returns the Vec<ManifestFile> — which the old manifest_file (singular, getter-shaped) didn't convey. The rustdoc captures both the "collect" and "also writes" aspects.
One small thing the rename missed: the comment at snapshot.rs:470-471 still refers to the old name twice ("Calling self.summary() before self.manifest_file() ... after self.manifest_file() returns"). Since the whole point of this PR is naming clarity, it'd be worth updating those two references to produce_manifest_file_list too so the comment doesn't contradict the method it describes.
Otherwise LGTM — only call site updated, no stray references elsewhere, CI green.
|
Thanks for catching those stale comments, a miss on my part! I've addressed those and merged in from |
|
@CTTY would you mind taking a look at this one? |
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Thanks for the PR Danny! I've left a comment
| /// Collects the list of manifest files to be included in the new snapshot. | ||
| /// | ||
| /// This method also writes the new manifests where required. | ||
| async fn produce_manifest_file_list<OP: SnapshotProduceOperation, MP: ManifestProcess>( |
There was a problem hiding this comment.
I'm thinking produce_manifest_files would better distinguish this from manifest list, which is a separate concept
Or produce_manifests, which is closer to java's terms. I'm happy with both!
There was a problem hiding this comment.
Thanks Shawn, I think your suggestion makes sense. I've updated to produce_manifests and clarified the Rustdoc comment a bit more.
3 participants
Which issue does this PR close?
What changes are included in this PR?
Renames the trait method and adds rustdoc.
The change is pretty small, although risks impacting open PRs. I would recommend to either merge now or park for a while.
Originally suggested as the naming does not indicate what the purpose of the method is.
Are these changes tested?
N/A