Split out convenience methods from ModuleWriter trait #2842
+217
−130
Conversation
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
e-nomem
commented
Nov 10, 2025
src/module_writer.rs
Outdated
Comment on lines
1522
to
1518
| let source = fs::read_link(file.path())?; | ||
| writer.add_file_with_permissions( | ||
| relative, | ||
| source.parent().unwrap(), | ||
| mode, | ||
| )?; | ||
| writer.add_file_infer_permissions(relative, file.path())?; |
Contributor
Author
There was a problem hiding this comment.
This is the only change I'm uncertain about. I'm passing file.path() here as the source so that the permission can be checked, but previously the code used to pass in the parent directory as the source. It's not clear to me why.
e-nomem
force-pushed
the
modulewriter-ext
branch
from
f56cc44 to
811f7fc
Compare
Copilot finished reviewing on behalf of
messense
November 11, 2025 11:06
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the ModuleWriter trait to improve its design by separating the core implementation method from convenience methods. The main method now accepts impl Read instead of byte slices, enabling streaming data processing.
- Split
ModuleWritertrait into a core trait with one method (add_data) and an extension trait (ModuleWriterExt) with convenience methods - Replaced byte slice parameters with
impl Readto support streaming - Introduced
StreamSha256for calculating hashes during streaming operations - Simplified permission handling to binary executable/non-executable flags (
0o755/0o644)
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/module_writer.rs | Core refactoring: split trait into ModuleWriter and ModuleWriterExt, changed API to use impl Read, added StreamSha256 struct for streaming hash calculation, simplified permission handling |
| src/source_distribution.rs | Updated all calls to use new add_data and add_file APIs with boolean executable parameter |
| src/build_context.rs | Updated imports to include ModuleWriterExt and adapted method calls to new API |
| tests/common/metadata.rs | Updated mock implementation to match new ModuleWriter trait signature |
| Changelog.md | Added changelog entry for the refactoring |
e-nomem
force-pushed
the
modulewriter-ext
branch
from
811f7fc to
e25fe76
Compare
e-nomem
force-pushed
the
modulewriter-ext
branch
2 times, most recently
from
6b3ad03 to
64f8ffb
Compare
messense
reviewed
Nov 13, 2025
e-nomem
force-pushed
the
modulewriter-ext
branch
3 times, most recently
from
006dfe4 to
5fe2cbe
Compare
e-nomem
force-pushed
the
modulewriter-ext
branch
from
5fe2cbe to
fd48219
Compare
Contributor
Author
|
@messense I can back out the change adding |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
I was thinking that the convenience methods with default implementations available on
ModuleWritershouldn't really be overridden and that implementers ofModuleWritershould only focus on the one primary method. In order to enforce that separation, this PR moves all the convenience methods into aModuleWriterExttrait that has a blanket implementation for allModuleWriters.In addition to that, the following related changes are included:
ModuleWriterto accept animpl Readinstead of a byte sliceimpl Read, add aStreamSha256struct that calculates the hash as the data is streamed to the underlying Writer0o755or0o644)The PR is structured so that the individual commits can be reviewed to ensure that there are no behavioral changes.