Separate tar archive and tar flags #492
Conversation
It used to have one flag (-r) to enable both tar archive and tar. Now it has two flags [ -r: for tar, -g: for tar archive]. Related to svenstaro#451
|
@svenstaro Could you take a look at this? Thanks in advance. |
|
Will do, sorry, pretty busy these days. |
There was a problem hiding this comment.
I really like this and would basically accept it as is code-wise but I'd really like to distinguish the terminology here: Let's call uncompressed tarballs uncompressed tar archives and compressed ones gz-compressed tar archives for the sake of being able to add on more compression methods later on without it being to confusing sounding.
Keep in mind: tar are always archives, whether they are compressed or not.
Use following terminology: uncompressed tarballs => `uncompressed tar archives` compressed ones => `gz-compressed tar archives`
| match self { | ||
| CompressionMethod::TarGz => tar_gz_enabled, |
There was a problem hiding this comment.
Come to think of it, it probably wasn't proper of me to go with CompressionMethod here as it should really have been ArchiveMethod from the beginning. Oh well, might change this after merging your PR. :)
Thanks for the suggestions and fixing the wrong terminology. It is now fixed in c9506c7. |
|
Cool stuff! Will merge right once CI passes. |
|
|
||
| sleep(Duration::from_secs(1)); | ||
|
|
||
| // Ensure the links to the tar archive exists and tar not exists |
There was a problem hiding this comment.
One small thing I just found:
// Ensure the links to the compressed tar archive exists and link to plain tar doesn't exist
There was a problem hiding this comment.
I am happy to fix this. But since you already merge this, should I still change this?
| assert!(parsed.find(Text).any(|x| x.text() == "Download .tar.gz")); | ||
| assert!(parsed.find(Text).all(|x| x.text() != "Download .tar")); | ||
|
|
||
| // Try to download, only tar_gz should works |
There was a problem hiding this comment.
// Try to download, only tar.gz should work
| @@ -51,3 +51,44 @@ fn archives_are_disabled(tmpdir: TempDir, port: u16) -> Result<(), Error> { | |||
|
|
|||
| Ok(()) | |||
| } | |||
|
|
|||
| #[rstest] | |||
There was a problem hiding this comment.
If you wanna do something cool here, you could use rstest for test parameterization to test -p and -g with very little test code overhead. Want to give that a try?
There was a problem hiding this comment.
Actually, on second thought, I want to more properly clean up these tests anyhow. I'll just do that after merging.
There was a problem hiding this comment.
I can try in maybe another PR?
There was a problem hiding this comment.
Sure, if you like, that'd be much appreciated. So what I'm thinking here: Have a single parametrized test in tests/archive.rs that would test for all possible combinations of compression flags whether the path is reachable whether the links are being rendered. Thanks to rstest, we'd end up with a bunch of separately generated distinct tests. I think that'd be rather clean and make quite a bit of sense. Wanna give it a shot?
Take a look at: https://docs.rs/rstest/0.7.0/rstest/attr.rstest.html#test-parametrized-cases
It used to have one flag (-r) to enable both tar archive and tar.
Now it has two flags [ -r: for tar, -g: for tar archive].
Related to #451