Fix empty folder missing problem when zip files. #330
Conversation
There was a problem hiding this comment.
Thank you for your contribution.
First, I'd prefer to have separate pull requests for unrelated changes.
About the empty dir handling of zip files. I think it's a good idea to add an option/parameter to let the user decide whether to include directory entries or not. Enabling it could be the default, since it's what most users would expect.
|
@lefou okay, let me separate this pull request to two. |
|
I'd like to do some "linting" works on #331 firstly. |
|
Let's just do the zip file changes, no need all the english grammar improvements and other random build config tweaks. |
os/src/ZipOps.scala
Outdated
| finally fis.foreach(_.close()) | ||
| } | ||
|
|
||
| private def makeZipEntry0( | ||
| sub: os.SubPath, | ||
| path: String, |
There was a problem hiding this comment.
Because treats a ZipEntry as directory if its name ends with "/" characters.
Firstly I want to add end "/" character, but in os.Path it's not valid.
There was a problem hiding this comment.
I know it's not a good way to implement, since it breaks the type check inside os.Path, could you give me some advice ?
should I rewrite another method to handle folders such like makeZipFolderEntry0 ?
There was a problem hiding this comment.
We should be able to keep the conversion to string and appending of / inside makeZipEntry0. is: Option should already tell us whether it's a file or directory I think
There was a problem hiding this comment.
Done. But I'm afraid This might be a little slightly fragile assumption, since it's relied on
val fis = if (os.isFile(file)) Some(os.read.inputStream(file)) else None
counter2015
force-pushed
the
empty-dir-zip
branch
from
856c749
to
1d1ff79
Compare
|
Does format checking relies on OS system ? In my local windows git bash environment one is 6 but another one is 253. |
|
In CI Test to simplify, |
|
@counter2015 yes adding a sort seems reasonable |
|
I found the test report depends on old commit, seems some weird caching issues in CI environment. For example. for new test code Maybe we should re-run the CI command ? |
| wd / "unzipped folder/nestedA", | ||
| wd / "unzipped folder/nestedB", | ||
| wd / "unzipped folder/one.txt", | ||
| wd / "unzipped folder/nestedA/a.txt", | ||
| wd / "unzipped folder/nestedB/b.txt" | ||
| ) | ||
| assert(paths.sorted == expected) |
There was a problem hiding this comment.
@lihaoyi I'm not sure why the expected output should be this, since the file tree likes following
.
├── File.txt
├── Multi Line.txt
├── emptyFolder
├── folder1
│ └── one.txt
├── folder2
│ ├── nestedA
│ │ └── a.txt
│ └── nestedB
│ └── b.txt
it seems missing the folder folder1 and folder2 level ?
counter2015
force-pushed
the
empty-dir-zip
branch
from
8b72ec1
to
e760565
Compare
|
@lihaoyi All test has passed excepted test on macos-latest, 11 It's stranged that the failed test cases seems not relative to this PR and might fails randomly. |
Fix: #328