Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(dif): Add ability to get server options from ChunkOptions #2302

Closed
wants to merge 2 commits into from

Conversation

szokeasaurusrex
Copy link
Member

Add functionality to the ChunkOptions trait, allowing the ChunkServerOptions to be obtained. With this change, DifUpload can no longer implement ChunkOptions directly, since it does not contain the server options, so we create a new ChunkedDifUpload struct to implement ChunkOptions by acting as a container for the DifUpload and ChunkServerOptions.

Also, we take advantage of the new functionality by refactoring upload_difs_chunked to take only a single argument, the ChunkedDifUpload. The upload methods now take owned options structs, since an upload should only be performed once.

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/proguard-upload-9 branch from f717d84 to a661c6c Compare December 5, 2024 12:31
Copy link
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

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

To be honest I don't see what this gets you other than reducing two function parameters into one

@szokeasaurusrex szokeasaurusrex marked this pull request as draft December 5, 2024 13:54
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/proguard-upload-8 branch from 99e7bae to 2c576c0 Compare December 5, 2024 15:33
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/proguard-upload-9 branch from a661c6c to 6203e7a Compare December 5, 2024 15:34
@szokeasaurusrex
Copy link
Member Author

To be honest I don't see what this gets you other than reducing two function parameters into one

imo the API is slightly nicer this way in the dependent PRs (#2303 and #2305). But, if you disagree, then I can undo this change, and rewrite those PRs accordingly

@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review December 5, 2024 15:38
@loewenheim
Copy link
Contributor

No, that's fair.

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/proguard-upload-8 branch from 2c576c0 to 1d5d593 Compare December 6, 2024 13:39
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/proguard-upload-9 branch from 6203e7a to d923964 Compare December 6, 2024 13:39
Base automatically changed from szokeasaurusrex/proguard-upload-8 to master December 6, 2024 13:54
Owned string is not necesary here, and having it will make it harder to convert `DifUpload` into `ChunkOptions`
Previously, `ChunkOptions` was implemented as a trait, but we realized it makes more sense to have it be a struct, instead. Now, as a struct, we also have `ChunkOptions` store the `ChunkServerOptions`, which will eventually allow us to simplify some API's, since we can take `ChunkOptions` only, rather than `DIFUpload` and `ChunkServerOptions` for methods such as `upload_difs_chunked`.
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/proguard-upload-9 branch from d923964 to cf02c17 Compare December 16, 2024 15:28
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.

2 participants