-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[WIP] Implement Path #3603
[WIP] Implement Path #3603
Conversation
Thanks! But if |
FWIW, [Python recently added something extremely similar]. It's just a lot more convenient to do stuff with objects instead of overly nested calls, and it also lets you do stuff like |
Pathname does in fact delegate many of its methods, but these are all just convenience methods. Its role is the representation of a path. This is a concept, which does not exist when using the functions from In ruby AFAIK, the File- and Dir functions which are working with filenames as arguments are trying to convert them with The benefit, as @kirbyfan64 pointed out, is the dealing with objects and methods instead of strings and functions. For visualization image a method # Settings
input_file = "./src/test.cr"
annotation_dir = "./annotations"
cache_dir = "/tmp"
# Classic
input_io = File.open(input_file)
cache_io = File.open(File.join(cache_dir, File.basename(input_file)))
annotation_io = File.open(File.join(annotation_dir, File.basename(input_file, ".cr") + ".html"))
# Pathname (assuming the 3 settings vars are Pathnames)
file = file.open
cache_io = (cache_dir / input_file.basename).open
annotation_io = (annotation_dir / input_file.basename.ext(".html")).open The only function which is handling files in the example is Maybe its a idea to implement a class As further convrnience we could add another class or shard which combines input_file = input_file.to_path
input_io = File.open(input_file.to_path)
cache_io = File.open(cache_dir.to_path / input_file.basename)
annotation_io = File.open(annotation_dir.to_path / input_file.basename.ext(".html")) |
Looking at the implementation, We already have This explains why we may be reluctant to add a 3rd solution, which is purely cosmetic and doesn't bring something new. |
I think there is value in Pathname abstraction. I like the type safety it can offer avoiding Strings if you want. But since there are some doubts to add it I would suggest to create a shard, have a mature implementation and if that end up better or more used we can think of adding it to std lib or to the organization if the author want it. |
I am personally not against it, for example the |
In java there's a |
I will personally about methods like That way, we avoid having to go over and update method signatures all over the place. |
@@ -3,6 +3,7 @@ require "c/stdio" | |||
require "c/stdlib" | |||
require "c/sys/stat" | |||
require "c/unistd" | |||
require "path" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fond that a core class like File
have to depend on another class to provide basic functionality like dirname
and others.
It also creates multiple instances of Path
which are discarded immediately.
I believe past comment in relation to rely was made in the opposite direction: that Path
relies on File
and Dir
classes and not the other way around.
Thanks for the feedback. Ill address it. Currently this is more an example for a The idea behind this is to replace strings when handling files and directories for comfort, type safety and maybe security (e.g. injection protection). Following this idea, the path-related What do you think about this? |
@@ -3,10 +3,9 @@ require "c/stdio" | |||
require "c/stdlib" | |||
require "c/sys/stat" | |||
require "c/unistd" | |||
require "path" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the require and then using it to define SEPARATOR
from Path::SEPARATOR
is not what I meant with my previous comment.
You're adding Path
as dependency of File
, not just by the require, but also all the methods owned by File now depend on an instance of Path
that is discarded when used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what you mean. I'm not sure if you have seen my last comment on the PR. I am proposing to move some of the File
methods which are currently just Path.new(path).xyz.to_s
to path and remove the source methods and the SEPARATOR*
constants from the source
I'm not very fond of this implementation. I like moving path related methods from But, It also loses some null bytes check, which reopens some very important security holes. Please keep them until we are sure we can remove them (spoiler: we will most certainly keep them). We can't expect a developer to have to instantiate a I would prefer a simpler solution that moves the |
(sorry, wrong button) |
We'll probably implement |
I started implementing Pathname. I plan to have almost no own code in here, besides some glue.
Is there a convention how to prefix macros in documentation? Like
.class_method
and#instance_method
I used%macro
for now.Any wishes or criticism is welcome. Do not fear, rebase and style will come later ;)