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

Add S3Path constructor that takes in S3Path and version #297

Merged
merged 18 commits into from
Jun 30, 2023
Merged

Add S3Path constructor that takes in S3Path and version #297

merged 18 commits into from
Jun 30, 2023

Conversation

hannahilea
Copy link
Contributor

Closes #294.

In this PR, fails if input path already has a version; could alternatively implement silently overwriting an existing version.

@hannahilea hannahilea marked this pull request as ready for review June 28, 2023 17:57
@ararslan
Copy link
Member

could alternatively implement silently overwriting an existing version

FWIW this is the behavior I would expect as a user. The same is true of config. It'd be nice more generally if S3Path(::S3Path; settings...) gave you back an S3Path with the provided settings, overwriting or populating as the case may be.

@hannahilea
Copy link
Contributor Author

FWIW this is the behavior I would expect as a user.

Iiiiiiinteresting.

It'd be nice more generally if...

This was actually the path I originally started down, before deciding that failure was the desired end state. Hrm.

src/s3path.jl Outdated Show resolved Hide resolved
src/s3path.jl Outdated
if !isnothing(path.version)
throw(
ArgumentError(
"Can't construct versioned path, input already specifies version: $path"
Copy link
Member

Choose a reason for hiding this comment

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

Why not allow the keyword version to overwrite the version of the S3Path silently? Should probably document the rational for this as part of the docstring

@omus
Copy link
Member

omus commented Jun 29, 2023

bors try

bors bot added a commit that referenced this pull request Jun 29, 2023
@bors
Copy link
Contributor

bors bot commented Jun 29, 2023

try

Build failed:

@hannahilea
Copy link
Contributor Author

Okay, I updated the approach here to be in line with @ararslan's expectations. Rereview required!

src/s3path.jl Outdated Show resolved Hide resolved
@ararslan
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jun 29, 2023
@bors
Copy link
Contributor

bors bot commented Jun 29, 2023

try

Build failed:

@ararslan
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jun 29, 2023
@bors
Copy link
Contributor

bors bot commented Jun 29, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

CHANGELOG.md Outdated Show resolved Hide resolved
hannahilea and others added 2 commits June 30, 2023 10:45
Co-authored-by: Alex Arslan <ararslan@comcast.net>
@hannahilea
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jun 30, 2023
@bors
Copy link
Contributor

bors bot commented Jun 30, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

Last minor changes

test/s3path.jl Outdated Show resolved Hide resolved
test/s3path.jl Outdated Show resolved Hide resolved
test/s3path.jl Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
hannahilea and others added 3 commits June 30, 2023 11:40
Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>
Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>
Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>
@hannahilea
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jun 30, 2023
@bors
Copy link
Contributor

bors bot commented Jun 30, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@ararslan
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 30, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 2052369 into JuliaCloud:master Jun 30, 2023
1 check passed
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.

Add S3Path constructor where you just pass in an S3Path and a version
4 participants