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

Writing to Data Links #9750

Merged
merged 53 commits into from
Apr 25, 2024
Merged

Writing to Data Links #9750

merged 53 commits into from
Apr 25, 2024

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Apr 19, 2024

Pull Request Description

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@radeusgd radeusgd self-assigned this Apr 19, 2024
@radeusgd
Copy link
Member Author

Converting to draft as some further work on copying files is needed

@radeusgd radeusgd marked this pull request as ready for review April 23, 2024 16:50
Comment on lines +240 to +245
copy_to : File_Like -> Boolean -> Any ! File_Error
copy_to self (destination : File_Like) (replace_existing : Boolean = False) = Data_Link_Helpers.disallow_links_in_copy self destination <|
if self.is_directory then Error.throw (S3_Error.Error "Copying S3 folders is currently not implemented." self.uri) else
Context.Output.if_enabled disabled_message="Copying an S3_File is forbidden as the Output context is disabled." panic=False <|
case destination.file of
destination_writable = Writable_File.from destination
case destination_writable.file of
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed Writable_File to File_Like to avoid eagerly triggering the conversion.

The problem was - converting a data link to Writable_File 'follows it' but if it did not exist, that will fail with File_Error.Not_Found and it would fail too early, not giving us a chance to abandon the operation because of disallow_links_in_copy.

Briefly I thought that this indicates that the Writable_File 'class' is too loaded and there should be a separation of concerns between expressing the capability of 'a file that can be written to' and resolving the data link.

But actually, almost all code does want this to happen in one step. Our users and library devs, will most of the time want to write a function taking Writable_File and be able to use this file to write their fancy data formats to it, not caring how the underlying structures resolve the data links to write to the data link targets - implementors of new formats should not care about data links, only about taking a Writable_File and writing to it.

Only implementors of new file systems need to be more careful around data links and for operations that may not want to follow the link (e.g. copy_to and move_to) they will need to avoid the eager conversion like here and use some other type (I decided to use File_Like as this is our 'most generic' type class that every File type should be implementing). I think that is fine as a bit more complexity in the filesystem logic sounds better than complicating the file format logic: we will likely end up with more formats than filesystems and external contributors/users will be more likely to need to write a new format than a new filesystem altogether. Moreover it's not hard anyway as one can just copy the 'best practices' from the S3_File implementation.

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Apr 25, 2024
@mergify mergify bot merged commit 0c989c6 into develop Apr 25, 2024
35 of 37 checks passed
@mergify mergify bot deleted the wip/radeusgd/9292-write-to-datalinks branch April 25, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Smarter logic when copying data links Writing to DataLinks
3 participants