-
Notifications
You must be signed in to change notification settings - Fork 3
Support .ELN-style crates in all zip readers and writers #258
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
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Not recursive and limited to 50 folders to avoid load vulnerabilities.
… deprecation linter errors
…AllUnsafe in entity builders
…tten We only want to add it anyway when reading, but avoid when reading a crate.
… in reader.ZipStreamStrategy
Notes
Conclusions
|
….ZipStreamStrategy
… reader.ZipStrategy
…pStreamStrategyTest
…view.saveAllToStream
…files-in-zip-streams fix: enforce correct subfolder name for additional files in StaticPreview.saveAllToStream
… Write accordingly. This eases importing reader and writer strategies of the same kind (source/destination type).
…WriteZipStreamStrategy
…rateWriter type parameter
Closes #251
See https://github.com/TheELNConsortium/TheELNFileFormat
From the view of RO-Crate Java these crates are zip crates with the special (incompatible) property that the crates are in a subfolder within the file.
This should be simple to add, but due to some weird Zip files, refactorings and deprecations this is a slightly larger task which adds also surprisigly larger amounts of code for testing and because I can't delete the deprecated code immediately.
Support reading
Reading is implemented in the strategies directly, as the only difference is that they need to search in subfolders. The FolderStrategy does not implement ELN support as it does not read zip files.
Support writing
Writers need to implement an additional interface which has a method (two, actually, for semantics) to turn a writer into ELN mode. The save method is not directly part of the strategy but of the CrateWriter. As not all writers will have this functionality, it stays a configuration in the strategy.
Other
writer.ZipStrategy and writer.ZipStreamStrategy have some very similar code. Consider making a utility class or base class.Postponed. Can be done later without API change.Make CratePreview generic?May be interesting in future, but for now I do not see a big advantage.Documentation
Notes to put into the documentation:
Deprecations
Breaking Changes
The Readers and writers now expose IOExceptions which must be handled.
I want this to have into the 2.x branch and therefore not want to break anything, but it turned out that the current implementation hid and logged errors to console when writing, similar to the rest of ro-crate-java. But as not every zip file is streaming-processable without a lot of memory, we need to provide a way to say "sorry, this is not going to work out". Also, reading and writing are important, high level tasks which should be handled by a developer using it.
Therefore, I consider this not a breaking change, but a fix. In practice, it likely breaks for users, but it will be easy to handle.
The methods to write an entity are now private methods in writer strategies. This is where they logically belong to. I do not consider it a breaking change because them being public was not intended anyway and just a matter of them being in the wrong place. So I consider it a fix regarding our approach to avoid invalid usage.
Amount of main code
Before
After / Current