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

Remove tar combinations from compression format #133

Conversation

SpyrosRoum
Copy link
Contributor

This allows us to remove a lot of duplicate code and make things simpler. It also doesn't remove any functionality as far as I can see.

@SpyrosRoum SpyrosRoum mentioned this pull request Oct 31, 2021
@marcospb19
Copy link
Member

marcospb19 commented Oct 31, 2021

It's been some time since I have been thinking about a rewrite of CompressionFormat, I planned on something like this:

pub struct Extension {
    compression_formats: Vec<CompressionFormat>,
    display_text: String, // or &str or OsStr or OsString, idk
}

This would allow us removing Tgz and Tlzma from that enum too.

So the extension .tgz would be made of:

Extension { compression_formats: vec![Tar, Gz], display_text: "tgz".into() }

Cause the code in this PR will likely break this piece of code:

let extensions_text: String = formats.iter().map(|format| format.to_string()).collect();

In this line, we get all the compression formats and use them to compose this string of extensions, so we can have the flexibility to change archive.gz.xz to archive.tar.gz.xz by just adding that .tar in the middle.

This requires the Display impl:

ouch/src/extension.rs

Lines 28 to 47 in a02eb45

impl fmt::Display for CompressionFormat {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(
f,
"{}",
match self {
Gzip => ".gz",
Bzip => ".bz",
Zstd => ".zst",
Lzma => ".lz",
Tar => ".tar",
Tgz => ".tgz",
Tbz => ".tbz",
Tlzma => ".tlz",
Tzst => ".tzst",
Zip => ".zip",
}
)
}
}

Of Extension to retrieve what was given in the CLI.

For example: If the user typed archive.tgz, and we trigger some error message with this extensions Display stuff, it would print archive.tar.gz instead, which can be confusing.

Ideas:

  1. Rework the Extension and CompressionFormat as I told in this comment.
  2. Change how we use extensions in error messages, having another system of "parsing the file_name of a filepath".
  3. Other?? idk

(See #88 too)

@SpyrosRoum
Copy link
Contributor Author

Is this about what you were thinking?
It touches a lot of sensitive places so I'd appreciate it if you could take a good look (we should really improve our tests soon).

@figsoda
Copy link
Member

figsoda commented Nov 1, 2021

or we could just not store the extension strings and get it from the original path every time we want to print an error

@marcospb19
Copy link
Member

marcospb19 commented Nov 2, 2021

or we could just not store the extension strings and get it from the original path every time we want to print an error

That's also possible, but in that case we would need come with another system to do the parsing and reformatting again, could happen, but may be harder to implement.

@marcospb19 marcospb19 merged commit ebe3918 into ouch-org:master Nov 2, 2021
@marcospb19
Copy link
Member

Merged with some warnings still, but I can fix those.

Maybe the missing_docs lints is too aggressive, idk.

@marcospb19
Copy link
Member

marcospb19 commented Nov 2, 2021

Oops, found one bug:

$ cargo run -q -- c src src.xz
[ERROR] Cannot compress to 'src.xz'.
 - You are trying to compress multiple files.
 - The compression format 'xz' cannot receive multiple files.
 - The only supported formats that archive files into an archive are .tar and .zip.

hint: Try inserting '.tar' or '.zip' before 'xz'.
hint: From: src.xz
hint:  To : src..tarxz

The last line:

hint:  To : src..tarxz

Should be src.tar.xz.

EDIT: is it really a regression tho? Or was my code there always wrong?

@SpyrosRoum
Copy link
Contributor Author

Well that case used to panic before so overall it's an improvement I'd say haha

@SpyrosRoum SpyrosRoum deleted the Remove-tar-combinations-from-CompressionFormat branch November 2, 2021 07:36
@marcospb19
Copy link
Member

Bug 2 comments above was fixed by #219 🙂.

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.

None yet

3 participants