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

Implement os:stat #1715

Closed
wants to merge 1 commit into from
Closed

Implement os:stat #1715

wants to merge 1 commit into from

Conversation

krader1961
Copy link
Contributor

Implement an os:stat command to provide detailed information about a file system path. This provides a more general API than builtins like path:is-dir and path:is-regular. Making it possible to test many other path attributes. I didn't deprecate those commands because they are used often enough it doesn't make sense to deprecate them and force users to use a more general mechanism (at least at this time).

In theory the os:stat command introduced by this change allows an Elvish implementation of something like the Unix ls, or Windows dir, command. However, my primary motivation is to make some unit tests I will introduce in a related change easier to write.

The introduction of a Time type serves only to make it possible to display filesystem timestamps. More functionality involving those timestamps depends on https://b.elv.sh/1030.

Related: #1659

Implement an `os:stat` command to provide detailed information about a
file system path. This provides a more general API than builtins like
`path:is-dir` and `path:is-regular`. Making it possible to test many
other path attributes. I didn't deprecate those commands because they
are used often enough it doesn't make sense to deprecate them and force
users to use a more general mechanism (at least at this time).

In theory the `os:stat` command introduced by this change allows an
Elvish implementation of something like the Unix `ls`, or Windows `dir`,
command. However, my primary motivation is to make some unit tests I
will introduce in a related change easier to write.

The introduction of a `Time` type serves only to make it possible to
display filesystem timestamps. More functionality involving those
timestamps depends on https://b.elv.sh/1030.

Related: elves#1659
@xiaq
Copy link
Member

xiaq commented Aug 7, 2023

I started working on an os:stat before seeing this PR.

Looking at this implementation, there are some things I prefer to do differently:

  • I'd prefer os:stat to be a relatively low-level API, sticking to exposing the information from the underlying Go data structure FileInfo.
    • This means no resolving UIDs and GIDs to names. Functionalities from os/user can be exposed separately.
    • I'd also rather stick to Go's modelling of setuid and setgid as separate from the perms bits. I don't like the Unix-centric perms bits either, but that is harder to fix.
  • I'd prefer os:stat to not be variadic. Just let it take one path.
  • I'd prefer to group information FileInfo.Sys() into a submap; otherwise it's too hard for the user to keep track of which fields are platform-dependent and which fields aren't.
  • An implementation issue: Exposing numbers that are potentially *big.Int as *big.Int is not correct: they should use int when they fit (see the long comment in pkg/eval/vals/num.go). This is important for low-overhead equality checks.

So I'll close this PR now.

@xiaq xiaq closed this Aug 7, 2023
@krader1961 krader1961 deleted the os-stat branch August 10, 2023 03:43
@krader1961
Copy link
Contributor Author

@xiaq: Will you be committing your own version of this change in the near future? I'm happy to make all of the changes you requested. The only feedback that is ambiguous is this one:

I'd prefer to group information FileInfo.Sys() into a submap; otherwise it's too hard for the user to keep track of which fields are platform-dependent and which fields aren't.

Are you suggesting that the platform dependent data be exposed in a os:stat structmap field that is itself a structmap that only includes attributes relevant for that platform? Thus eliminating attributes from the os:stat structure that are always the zero value for a particular platform? Doing that means that referencing an attribute that is always the zero value on a platform will result in a key lookup exception. That may be preferable to the behavior of my implementation which silently returns a predictable, constant, value on that platform. I'm inclined to agree that the added complexity is worthwhile to avoid executing code on a platform that does not support the desired semantics. I just want to confirm that is what you meant by your feedback.

@xiaq
Copy link
Member

xiaq commented Aug 10, 2023

I will commit my implementation soon.

Re FileInfo.Sys(): I meant exposing it as a separate sys field which is itself another map, rather than as fields of the stat map itself. The keys of the submap will be system-dependent.

For example, on Linux you'd have os.stat returns a map [&sys=[&blk-count=... ...] ...], but on Windows, it will be [&sys=[&file-attributes=...] ...].

Also FWIW, I made a change a while ago that made structmaps indistinguishable from maps to Elvish code (96752af). Pseudo-maps are still a thing.

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.

2 participants