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

Make S3Path a concrete type, with the config as a field only not a type param #240

Open
nickrobinson251 opened this issue Feb 15, 2022 · 0 comments

Comments

@nickrobinson251
Copy link
Contributor

the type param was added in #140

but this change makes S3Path non-concrete, which in practice makes working with S3Paths much more painful
e.g. one has to dispatch on Vector{<:S3Path} rather Vectpr{S3Path} (so was breaking), and can also lead to methods being compiled (specialised) on the Config type which is usually of no importance. instead, the Config could just be a field, which we can check when necessary, and in this context dynamic dispatch is probably fine (#140 (comment))

in my experience of maintaining a (private) codebase using S3Paths, the addition of th type parameter has been a continuing source of pain. since the same flexibility can be supported without the Config being stored at the type level, i suggest we remove it (in the next breaking release)

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

No branches or pull requests

1 participant