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

Deprecate Path#dirname #7762

Closed
wants to merge 3 commits into from

Conversation

j8r
Copy link
Contributor

@j8r j8r commented May 10, 2019

Here is the use case I faced, how to get the directory name?

With either
File.basename(File.dirname(Dir.current))
or
Path.new(Path.new(Dir.current).dirname).basename.to_s
or a mix of both.

One can also use this, but this isn't as clear nor as efficient:
Path.new(Dir.current).parts.last.lchop

Now, one can do:
Path.new(Dir.current).dirname.basename.to_s
and the root directory of the current one:
Path.new(Dir.current).dirname.dirname.basename.to_s

If a String is wanted, #to_s can still be called.


EDIT: There is already Path#parent, deprecating Path#dirname

Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

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

I think that makes sense, the dirname still is a path. Didn't see Path#parent

@j8r
Copy link
Contributor Author

j8r commented May 10, 2019

I'am confused, what's the difference between Path#parent and Path#dirname?
Path.new(Dir.current).parent.basename seems to do what I'm aiming for.

Edit: that's the same, dirname can be replaced by parent.to_s with this PR. Do we deprecate Path#dirname?

@asterite
Copy link
Member

@j8r I guess dirname is just parent.to_s, in case you need the parent as a string.

I do agree that it might be confusing.

So in your case you can do Path.new(Dir.current).parent.basename`, right?

I'd also like to see Path.current, which is simply Path.new(Dir.current).

@bew
Copy link
Contributor

bew commented May 10, 2019

I'd also like to see Path.current, which is simply Path.new(Dir.current)

With Path.currentit's not clear that it's the current dir (even if it's the only possibility), maybe Path.current_dir ?

@j8r
Copy link
Contributor Author

j8r commented May 10, 2019

But having Dir.current inside Path goes against your ideas #7688 that Path is only a wrapper that doens't perform any syscall. This is another topic.

I've added a deprecation for Path#dirname, waiting for more thoughts.

@j8r j8r changed the title Make Path#dirname return Path Deprecate Path#dirname May 10, 2019
@bew
Copy link
Contributor

bew commented May 10, 2019

True @j8r, another solution would be to make Dir.current return a Path directly, so Path doesn't have to do the call itself

Or add Dir.current_path...

@j8r
Copy link
Contributor Author

j8r commented May 10, 2019

No @bew, because:

  • no File and Dir return Path. Some methods are accepting Path, and there are some issues
  • this would be a breaking change
  • if a new method is added, it can be in Path
  • this isn't related to the issue, the main point was Path#dirname, Dir.current was only an example. I could have pick "/var/log" instead.

src/path.cr Outdated Show resolved Hide resolved
@asterite
Copy link
Member

I vote to close this. There's nothing wrong with parent and dirname.

@j8r
Copy link
Contributor Author

j8r commented May 10, 2019

I think it's confusing, others can be too like me. Any good reason to have both?

@asterite
Copy link
Member

One returns a string, which is familiar and known from File.dirname, the other one returns a Path. Why force users to write to_s when they can simply get something with one method call?

@j8r
Copy link
Contributor Author

j8r commented May 10, 2019

The user that is familiar with File.dirname, can use File.dirname?
parent.to_s vs dirname... for me the result is the same, I was using Path[mypath.dirname] because I was familiar with it, and doesn't know an other twin method returning Path existed.
I may be alone on this, I admit.

Co-Authored-By: Sijawusz Pur Rahnama <[email protected]>
@straight-shoota
Copy link
Member

Yes, the result of path.dirname is equivalent to path.parent.to_s. So it's essentially a short cut. One could argue whether it's worth having this method. And this was in fact brought up during the review of #5635. But in the end, it was agreed on the current API. And I'd like to keep it that way - at least for now. The Path API is brand new and still needs to be integrated with other APIs. I'd like to put more effort into this than disucussions about whether to have single method which doesn't make a big difference either way.
We can surely revisit the Path API, but please let's gather some some experience in using it first.

File.dirname is supposed to go away, so this is not an alternative.

@j8r
Copy link
Contributor Author

j8r commented May 12, 2019

Maybe one day all paths would be represented as Path instead of String.
For the time being, String is the most common denominator in the stdlib, I use Path#to_s very (too) often.

@j8r j8r closed this May 12, 2019
@j8r j8r deleted the make-path-dirname-return-path branch May 12, 2019 17:36
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.

6 participants