Allow copying File Options Again

[wyatt-herkamp]

When extra_data and central_extra_data were added to FileOptions we lost the ability to implement Copy for FileOptions.
Plenty of people will want to Zip a basic set of files. But now we have an unnecessary Atomic value being incremented and decremented. Also an empty allocation inside the Vec

So this solution has FileOptions take a Generic Parameter to hold "Extra Options" This can be the Unit type or ExtendedFileOptions

This also puts extra_data and central_extra_data in an Option inside ZipFileData removing the Atomic usage in some cases there too.

This does break the current code as people will need to specify the generic parameter or change FileOptions to either SimpleFileOptions or FullFileOptions

I also corrected the write_dir test to remove the deprecated API calls

[Pr0methean]

Mostly looks good, but I have some nitpicks.

[Pr0methean]

Almost perfect! I prefer methods to return a borrow rather than a clone, so that the caller can decide whether to clone it.

[Pr0methean]

The build currently fails; could you please fix it? You may need to build with --all-features and with --no-default-features to reproduce the errors.

Also, this PR raises a Clippy error:

error: using `clone` on type `FileOptions<()>` which implements the `Copy` trait
Error:    --> examples/write_dir.rs:106:47
    |
106 |             zip.add_directory(path_as_string, options.clone())?;
    |                                               ^^^^^^^^^^^^^^^ help: try removing the `clone` call: `options`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
    = note: `-D clippy::clone-on-copy` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::clone_on_copy)]`

[Pr0methean]

Please add #[allow(unknown_lints)] as well, since this lint didn't exist in 1.70 (the current MSRV).

[Pr0methean]

Hi,

It turns out that having a public trait extend a private trait was actually an error until Rust 1.74 (rust-lang/rust#113126, https://doc.rust-lang.org/error_codes/E0445.html) which has only been stable since November. Is there another way you can implement the sealing so that we don't have to bump the MSRV that far?

[Pr0methean]

This turns out not to be allowed before 1.74.0, and the current MSRV is 1.70.0. To fix this, make Sealed a public member of a private module; see https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/#sealing-traits-with-a-supertrait and https://doc.rust-lang.org/error_codes/E0445.html#.

[wyatt-herkamp]

Hi,

It turns out that having a public trait extend a private trait was actually an error until Rust 1.74 (rust-lang/rust#113126, doc.rust-lang.org/error_codes/E0445.html) which has only been stable since November. Is there another way you can implement the sealing so that we don't have to bump the MSRV that far?

I think this works

[Pr0methean]

Thanks; this will be merged automatically provided the tests pass.

[Pr0methean]

Whoops, looks like some of your commits aren't signed! I'd suggest squashing the PR so that you only have to sign one.

[wyatt-herkamp]

Please stop merging every commit into my PR.

A. I rather use Rebase
B. That was a completely irrelevant file that does not require merging.

If you are not doing that. Then what is Github doing? I have never had a PR auto-merge commits like this.

[Pr0methean]

After your squash and force-push, I'm still seeing:

"Merging is blocked
The base branch requires all commits to be signed. Learn more about signing commits."

This indicates that GitHub still doesn't recognize your signature. When it's recognized, you'll be able to see a Verified badge on the commit on the source branch's page.

[Pr0methean]

This repo's main branch has the "Require branches to be up to date before merging" rule, to prevent merging two PRs that were compatible with their last common ancestor but not with each other (e.g. because they add different new functions with the same name).

[wyatt-herkamp]

After your squash and force-push, I'm still seeing:

Yeah I had done it in two separate actions. I have never setup Git Signing before. So It took me a moment.

[Pr0methean]

Please don't close and reopen the PR unless you've rebased it, because then the CI checks are ignored and I have to manually approve them to start again. (If you've rebased it, then it's fine, because I'd have to restart them anyway.)

[Pr0methean]

It looks like displaying a "Rebase branch" button instead of "Update branch" is in a lot of people's wish lists: https://github.com/orgs/community/discussions/3245 In future, I'll try to remember to check whether rebasing is an option; but it may not be if it would invalidate your signatures. (You'd think that if that happened, GitHub would be able to replace the signatures with ones of its own; not sure why that isn't implemented.)

[wyatt-herkamp]

It looks like displaying a "Rebase branch" button instead of "Update branch" is in a lot of people's wish lists: https://github.com/orgs/community/discussions/3245 In future, I'll try to remember to check whether rebasing is an option; but it may not be if it would invalidate your signatures. (You'd think that if that happened, GitHub would be able to replace the signatures with ones of its own; not sure why that isn't implemented.)

I just wish it would block it. Let me handle the merge or rebase. Don't do it automatically.

[Pr0methean]

That means the CI can't run until you've rebased and I've approved the rebase to have the CI run on it, so PRs would take even longer to be merged. Plus, I may not be able to check PRs daily in the future -- I'm currently on leave from work, but will be either returning to full-time work or searching for a new job starting in early May, which may cause further delays. This is especially true if https://github.com/orgs/community/discussions/119719 and https://github.com/orgs/community/discussions/119723 turn out not to be the last issues of their kind that I'll get temporarily stuck on, or if #26 and #27 aren't the last config changes I'll discover a need for while others' PRs are waiting on CI. I'm fairly new at receiving PRs from other humans on a repo in parallel with each other and with my own work on the same repo, so there may be a lot of gotchas I've yet to discover. Please continue to be patient.

[Pr0methean]

Thanks! This is now merged, and I expect to release it in version 1.1.0 within the next few days.