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

[feature] output in ZIP format if extension is '.zip' #788

Merged
merged 11 commits into from
Jun 29, 2021
Merged

Conversation

woshilapin
Copy link
Contributor

@woshilapin woshilapin commented Jun 25, 2021

First commit is only about setting up some tests.
Second commit is about the new feature.

The new feature is to detect if --output contains .zip in the name. If no, the previous behavior is done: writing files to a folder. If yes, then we write the output files to a temporary folder, then ZIP all these files into the --output path.

🔍 Help for reviewing:

  1. 8ec61ef: add tests on the binary for gtfs2ntfs
  2. 7540763: create a ZIP file in gtfs2ntfs if output path has extension .zip
  3. a6ef370: (1) add write() and write_to_zip() functions on the API of transit_model::netex_france and (2) create a ZIP file in gtfs2netexfr and ntfs2netexfr if output path has extension .zip
  4. e8743b1: create a ZIP file in ntfs2ntfs if output path has extension .zip
  5. f79827b: create a ZIP file in ntfs2gtfs if output path has extension .zip and add a write_to_zip() function to transit_model::gtfs

🏗️ All the steps that have to be implemented

  • gtfs2ntfs supports ZIP export (7540763)
  • gtfs2netexfr supports ZIP export (a6ef370)
  • ntfs2netexfr supports ZIP export (a6ef370)
  • ntfs2ntfs supports ZIP export (e8743b1)
  • ntfs2gtfs supports ZIP export (f79827b)
  • Bump versions of Cargo.toml everywhere (1e57620)

❓ Unresolved questions

  • Tests added in gtfs2netexfr and ntfs2netexfr are covering more than what is in tests/write_netex_france.rs. Should I remove them from tests/write_netex_france.rs? -> On the binary tests, only test that output files are produced (not the entire output fixture) and extend to all the other similar tests (f2af969, 0ee7806, 0ee7806, b7e7f58)
  • Should we remove transit_model::netex_france::Exporter from the public API? -> Make Exporter private and possibly document the migration to the new write() function (1e57620)
  • Should we homogenize the ntfs::write(model: &Model, ...) and gtfs::write(model: Model, ...) (some with reference and some other with ownership)? -> To be done in another PR
  • Add a test for --output with an extension that is not .zip (585b37e)

@woshilapin woshilapin requested a review from antoine-de June 25, 2021 14:39
patochectp
patochectp previously approved these changes Jun 25, 2021
antoine-de
antoine-de previously approved these changes Jun 25, 2021
Copy link
Contributor

@antoine-de antoine-de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉
it's a pity that the zip file is not done using directly a zip archive writer but it's fine for me to do this in a separate PR 😁

@woshilapin
Copy link
Contributor Author

tada
it's a pity that the zip file is not done using directly a zip archive writer but it's fine for me to do this in a separate PR grin

Compared to writing to a temporary folder, then into an archive, it sure would be better. But not sure that it'd be as simple to do because it would force us to call ZipWriter::start_file() in code that doesn't know (and doesn't have to know) that it is a ZIP or not. We might need an abstraction here... in a separate PR for sure 😛

datanel
datanel previously approved these changes Jun 28, 2021
Copy link
Member

@datanel datanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved but -1 for the "pouet" in the fixtures.

patochectp
patochectp previously approved these changes Jun 29, 2021
patochectp
patochectp previously approved these changes Jun 29, 2021
@woshilapin woshilapin merged commit 7dbad03 into master Jun 29, 2021
@woshilapin woshilapin deleted the output-zip branch June 29, 2021 15:06
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.

4 participants