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

/ method is a pun #53

Closed
JeffBezanson opened this issue Nov 6, 2019 · 7 comments · Fixed by #75
Closed

/ method is a pun #53

JeffBezanson opened this issue Nov 6, 2019 · 7 comments · Fixed by #75

Comments

@JeffBezanson
Copy link

Joining paths is not division, so I think this method should be removed.

@rofinn
Copy link
Owner

rofinn commented Nov 6, 2019

FWIW, it isn't really a pun... it's just the most common separator used for various filesystems and is generally ubiquitous. Multiple other languages use this operator for joinpath, so this isn't completely off base, IMHO. Please see the discussion in which various other joinpath operators were considered #2. If FilePathsBase is ever consider as a stdlib then I'll re-open the discussion around this and a few other API choices, but until then I don't really have a strong inclination to change it.

NOTE: I'm pretty opposed to using the string concatenation operator.

@rofinn rofinn added the wontfix This will not be worked on label Mar 30, 2020
@rofinn rofinn closed this as completed Mar 30, 2020
@JeffBezanson
Copy link
Author

Of course I can't make you change this, but note that it very much is a pun, because Base / means division. That's what an operator pun means in the context of julia. Outside the context of julia Base.:/, yes of course / is commonly a path separator.

Another option to consider is providing a different / function from Base, so that users can lexically opt-in:

/(args...) = Base.:/(args...)
/(root::AbstractPath, ...) = # join paths

Then only code that does import FilePathsBase: / sees this definition, instead of extending all / call sites.

@rofinn
Copy link
Owner

rofinn commented Mar 31, 2020

Alright, I guess I have a few questions then:

  1. Doesn't * have the same problem then because it can mean either multiplication or concatenation depending on the types?
  2. Do you know if there is an existing precedent for your proposed solution? Overwriting (vs extending) the base function in a user's scope seems a bit counter-intuitive, but if this is a known pattern then I'd be willing to change and document the alternate approach.
  3. Is there any other issue with extending the base method, outside inconsistent meanings, for types we control here (e.g., performance concerns, sources of bugs)?

@rofinn
Copy link
Owner

rofinn commented Apr 8, 2020

I think it would probably be easier to just replace the operator (e.g., </> which is what haskell does).

@JeffBezanson
Copy link
Author

Answers:

  1. Strings form a ring with concatenation as the * operator, and this convention is used e.g. in math and CS theory. But if we were starting over I think we'd probably do this differently since it hasn't been too popular.
  2. Indeed, this approach doesn't seem to be used too often but I'm not sure why. I think people are used to the idea of effectively a single global namespace for things like operators.
  3. Compiler performance. When the compiler sees a/b without knowing types, it has to consider that the code might call FilePathsBase, when in fact that is unlikely since the vast majority of uses of / aren't path operations. So this relates to the semantic issue in an interesting way; keeping function meanings consistent gives more information to the compiler about which bits of code are related.

@rofinn
Copy link
Owner

rofinn commented Apr 8, 2020

Hmmm, okay, thank you for the clarification. Alright, before I change this is there any other operator you'd recommend as I can't seem to define things like </> either?

@rofinn rofinn removed the wontfix This will not be worked on label Apr 8, 2020
@rofinn rofinn closed this as completed in #75 Apr 8, 2020
@rofinn
Copy link
Owner

rofinn commented Apr 9, 2020

Unfortunately, option 2 no longer works with nightly: #75 (comment)

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 a pull request may close this issue.

2 participants