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

Reconsider overloading Base.join #58

Closed
KristofferC opened this issue Jan 27, 2020 · 6 comments
Closed

Reconsider overloading Base.join #58

KristofferC opened this issue Jan 27, 2020 · 6 comments

Comments

@KristofferC
Copy link

Base.join has a clear docstring which is to join an array of strings with delimiters. This seems like a different operation than what the join on types in FilePathsBase provides. Base already has a function that has the intended meaning which is joinpath. So, to me, just removing overloading join and just using joinpath would be a good idea.

@kmsquire
Copy link

To add to this:

  • Base.realreturns the real part of a complex number
  • Base.abs takes the absolute value of a number
  • Base.size gives the dimensions of an array

There's no problem using those in your package if you don't export them (or don't override the Base version), but the rule of thumb is that if they override something in Base, they should refer to the same concept.

@rofinn
Copy link
Owner

rofinn commented Jan 31, 2020

I don't see a strong reason why the same function name can't support multiple concepts as long as both concepts are explained in the docstring, which they are. I think it's pretty clear based on the arguments what these methods do and how they differ from other base methods. I suppose an ideal solution would be if Julia supported dynamic method merging, so that we don't need to explicit extend base to avoid conflicts. In any case, I'm not inclined to change the API of this package just to hack around a current limitation of the language.

@rofinn rofinn closed this as completed Jan 31, 2020
@KristofferC
Copy link
Author

FWIW, the fact that functions have meaning (and are not just random names in a namespace that can be arbitrarily merged) Is pretty fundamental for generic programming. This is not a limitation of the language and it is exactly the same in other programming languages with generic concepts. For example C++ has the same recommendation

Use overloaded functions (including constructors) only if a reader looking at a call site can get a good idea of what is happening without having to first figure out exactly which overload is being called.

Or in the Julia docs

Thus, the overall behavior of a function is a patchwork of the behaviors of its various method definitions. If the patchwork is well designed, even though the implementations of the methods may be quite different, the outward behavior of the function will appear seamless and consistent.

Anyway, just opened this issue in case of these extension being mostly by accident

@kmsquire
Copy link

kmsquire commented Jan 31, 2020

Okay, it's your package. 👍

One of the advantages of using the same method names (and interfaces) is that 3rd parties could easily switch between Base.Filesystem and FilePaths.jl (with no code change).

You do provide aliases for most of the common functions (mtime and ctime, at least, are missing), although you don't advertise that fact.

Would you consider making that more obvious in FilePaths.jl, and providing aliases for the rest of the functions?

(I'd be willing to make a PR...)

@rofinn
Copy link
Owner

rofinn commented Jan 31, 2020

Anyway, just opened this issue in case of these extension being mostly by accident

Thanks, yeah, it was an intentional compromise when designing the API. I don't mean to be rude by disagreeing or closing. My view was that it seems redundant to writing things like joinpath when you're explicitly passing in a path type. You might also not like that we overload / as a joinpath operator (it's actually my preferred option now)?

One of the advantages of using the same method names (and interfaces) is that 3rd parties could easily switch between Base.Filesystem and FilePaths.jl (with no code change).

Yes, that was the idea. For example, I believe joinpath also works if folks would rather use that.

You do provide aliases for most of the common functions (mtime and ctime, at least, are missing), although you don't advertise that fact. Would you consider making that more obvious in FilePaths.jl, and providing aliases for the rest of the functions?

👍 Yes, if you feel like there is missing functionality or aliases for Base / Filesystem functions please feel free to open a PR to FilePaths.jl (if it's low-level enough it might belong here). There is already some code there to help with switching (e.g., a macro for writing APIs that can operation on both strings and filepaths).

@rofinn
Copy link
Owner

rofinn commented Apr 8, 2020

I still feel like a single function can support multiple concepts based on the context of how its called (e.g., types passed in), but since it seems unlikely that julia will support method merging in the future I'll just change it and move on.

Changes:

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

3 participants