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

Remove FileInfo.path #1770

Closed
bartlomieju opened this issue Feb 13, 2019 · 11 comments · Fixed by #2313
Closed

Remove FileInfo.path #1770

bartlomieju opened this issue Feb 13, 2019 · 11 comments · Fixed by #2313

Comments

@bartlomieju
Copy link
Member

Discussion from gitter chat.

Calls to stat and lstat return FileInfo with null values for name and path.

const fileInfo = deno.stat("./log.txt");
fileInfo.name === null;
fileName.path === null;

Call to readDir returns FileInfo[] with name and path filled.

const dirInfo = deno.readDir(".");
const fileInfo = dirInfo[0];
fileInfo.name === "<filename>";
fileName.path === "<file path>";

stat does not return values for those two fields, which might lead to confusion down the road. It'd be better not to do additional syscall during stat.

CC @ry

@ry
Copy link
Member

ry commented Feb 13, 2019

Also why do we have both name and path in FileInfo?

deno/js/file_info.ts

Lines 30 to 34 in e782ba1

/** Returns the file or directory name. */
name: string | null;
/** Returns the file or directory path. */
path: string | null;

@bartlomieju
Copy link
Member Author

Isn't it like this?

/var/log/postgres/log.txt
name === "log.txt"
path === "/var/log/postgres/log.txt"

@ry
Copy link
Member

ry commented Feb 13, 2019

That seems like something people can compute themselves. We shouldn't be doing more than the minimal amount of work.

Note the structure of https://golang.org/pkg/os/#FileInfo

@bartlomieju
Copy link
Member Author

Golang basically removes leading directory and trailing slashes from name given to stat.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 13, 2019

As @GrosSacASac pointed out in Gitter, these could be memoized getters, so the first time they are accessed they are calced/retrieved and stored. Of course it maybe just better to have them as functions and people then understand there is some sort of cost to retrieving them. It is certainly doable with getters, but users don't consciously realise there is a cost.

@hayd
Copy link
Contributor

hayd commented Feb 14, 2019

I think we can stub both these in on both stat and lstat calls, since we're passed the filename (.path) and can take the .name from that too.

b723ea3

@arcatdmz
Copy link
Contributor

arcatdmz commented May 8, 2019

I personally prefer retaining the path property since functions like this can be simpler without the first filePath argument.

Meanwhile, given that #1774 was closed without getting merged, it seems like the conclusion is (unfortunately) already made that we're removing these properties from FileInfo. If so, this issue could also be closed.

@hayd
Copy link
Contributor

hayd commented May 8, 2019

Unfortunately I think this logic needs to be moved to something in std. I don't really understand @ry's decision as I think it's useful, trivial and without perf impact, but looks like this convenience needs to be wrapped around every fs function in std (with a FileInfo type defined there which includes both path/name fields). 🙄

@ry
Copy link
Member

ry commented May 8, 2019

FileInfo doesn't need both the base name and the path. It's redundant.

If you want to have Stat copy the base name into the FileInfo, I'd be fine with that. But first, FileInfo.path needs to be removed.

@arcatdmz
Copy link
Contributor

arcatdmz commented May 9, 2019

I thought stat and lstat resolve the provided path inside their implementation anyway, and returning the resolution result (full path) as the path property does not have any perf impact.

If things are not like this (it seems like so), I'm totally convinced that FileInfo.path can be removed.

@ry
Copy link
Member

ry commented May 13, 2019

I am removing FileInfo.path
denoland/std#398
#2313

@ry ry changed the title Confusing FileInfo interface Remove FileInfo.path May 13, 2019
@ry ry closed this as completed in #2313 May 16, 2019
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.

5 participants