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

Allow manual specification of the ZIP file structure #51

Closed
wants to merge 6 commits into from

Conversation

AshesITR
Copy link
Contributor

@AshesITR AshesITR commented May 9, 2020

fixes #50

TODOs:

  • update documentation
  • write tests for new functionality

I did a major rewrite of all the get_zip_data related functions to reduce duplication and rewrote all tests that previously directly used those functions.

All tests pass on my machine (R 4.0.0, Windows 10)

I didn't update the zip documentation yet, but the feature should be complete.

TODO: update documentation
@AshesITR
Copy link
Contributor Author

AshesITR commented May 9, 2020

The travis failure looks like file.path("a", "") behaves differently under linux.
Can anyone confirm this?
Under Windows we have file.path("a", "") == "a"...

@AshesITR
Copy link
Contributor Author

@gaborcsardi Do you have access to a linux environment by chance?
Otherwise, it'll probably be hit-and-miss until I figure out what exactly goes wrong...

@codecov-io
Copy link

codecov-io commented May 14, 2020

Codecov Report

Merging #51 into master will decrease coverage by 0.20%.
The diff coverage is 93.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   83.53%   83.33%   -0.21%     
==========================================
  Files           9        9              
  Lines         498      480      -18     
==========================================
- Hits          416      400      -16     
+ Misses         82       80       -2     
Impacted Files Coverage Δ
R/process.R 92.42% <50.00%> (ø)
R/utils.R 98.48% <100.00%> (+2.05%) ⬆️
R/zip.R 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 149e11b...1812ed0. Read the comment docs.

@gaborcsardi
Copy link
Member

Thanks! Sorry for the delay, I'll look at this soon.

@AshesITR
Copy link
Contributor Author

AshesITR commented May 15, 2020

AppVeyor failed with zip_process() error code 10 (EOPENWRITE).
Not sure what is causing this...

@gaborcsardi Can you trigger a re-build to ensure this is not a random failure? Thanks!

@gaborcsardi
Copy link
Member

I am sorry, but I just realized that we cannot do this, at least not like the current proposal. The problem is that names in lists and vectors are internalized in R, i.e. they are treated as symbols of the language internally.

R can only store symbols in its native encoding. Effectively this means that e.g. on Windows, where the encoding is something like latin-1 internally, R cannot have symbols in Unicode.

In general storing file or path names as names of vectors or lists in not a good idea.

Maybe we could URL-encode the file names? But that's not exactly intuitive. Or have some other way of specifying the structure, without relying on names?

@AshesITR
Copy link
Contributor Author

That's unfortunate.

We could easily add a keys argument to zipr et al.
It would have to be last to remain compatible with code using positional arguments, but the implementation would be straight-forward.

@gaborcsardi
Copy link
Member

Yeah, I think a keys argument is probably the best.

@AshesITR
Copy link
Contributor Author

The PR looks good to me now.
Incorporating the fix for #45, my local machine sucessfully runs all Windows tests successfully.
Do you need anything else before merging?

@AshesITR
Copy link
Contributor Author

@gaborcsardi Let me know if you want me to resolve the merge conflicts.
NB I won't be able to update the documentation due to roxygenlabs.

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.

Feature: allow manual specification of the ZIP directory structure
3 participants