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

Expose more Go os package functions (e.g., os.Remove) #1659

Open
krader1961 opened this issue Feb 18, 2023 · 2 comments
Open

Expose more Go os package functions (e.g., os.Remove) #1659

krader1961 opened this issue Feb 18, 2023 · 2 comments

Comments

@krader1961
Copy link
Contributor

krader1961 commented Feb 18, 2023

While rewriting my Bash scripts into Elvish to automate tasks, such as building and testing Elvish on all my systems, I noticed I was running this shell command on my primary development system: rm -f elvish. It shouldn't be necessary to run an external command to do that. Yet I couldn't find an existing Elvish builtin to do the same thing. So I am opening this issue to propose that the Go os package be exposed (not necessarily in its entirety) as an Elvish module. Alternatively, we might want to implement portions of the os package in other Elvish modules; e.g., the file or path module. I am ambivalent about which approach is preferable since a precedent has already been established by file:open wrapping the Go os.Open function. We'll probably want to continue building on that precedent by adding a path:remove builtin that wraps the Go os.Remove function. However, I feel this needs some discussion before anyone creates a pull-request.

@krader1961
Copy link
Contributor Author

Note that resolving this issue also enables resolving issue #1661 since it would allow replacing a lot of external commands used by the epm module with Elvish builtins.

krader1961 added a commit to krader1961/elvish that referenced this issue Mar 27, 2023
I considered adhering more closely to the traditional Unix `rm` and
`rmdir` commands. For example, by implementing each independently and
supporting the `--force` option. However, I concluded there are
insufficient reasons to do so. In particular, the POSIX `--force` (or
`-f`) option is most often used solely for its side-effect of making
elimination of a non-existent path a non-error. I also don't like that
`-f` also fixes permission errors to allow allow removing files. If we
decide that feature is needed it should be added under a distinct option
(e.g. `&fix-perms`).

Related: elves#1659
krader1961 added a commit to krader1961/elvish that referenced this issue Mar 29, 2023
krader1961 added a commit to krader1961/elvish that referenced this issue Mar 30, 2023
krader1961 added a commit to krader1961/elvish that referenced this issue Apr 4, 2023
I originally started work on this to provide a mechanism to verify the
behavior of other unit tests; e.g., a `path:chmod` command I'm working on.
Primarily because the external `stat` command has different option names
depending on whether you're on a BSD or Linux platform making it hard to
use portably. However, since it seems like this could be generally useful I
went ahead and implemented support for the metadata that is found on some,
not all, platforms.

TBD is providing better support for interpretating the `mode` value.

Related: elves#1659
krader1961 added a commit to krader1961/elvish that referenced this issue Apr 4, 2023
I originally started work on this to provide a mechanism to verify the
behavior of other unit tests; e.g., a `path:chmod` command I'm working on.
Primarily because the external `stat` command has different option names
depending on whether you're on a BSD or Linux platform making it hard to
use portably. However, since it seems like this could be generally useful I
went ahead and implemented support for the metadata that is found on some,
not all, platforms.

TBD is providing better support for interpretating the `mode` value.

Related: elves#1659
krader1961 added a commit to krader1961/elvish that referenced this issue Apr 4, 2023
I originally started work on this to provide a mechanism to verify the
behavior of other unit tests; e.g., a `path:chmod` command I'm working on.
Primarily because the external `stat` command has different option names
depending on whether you're on a BSD or Linux platform making it hard to
use portably. However, since it seems like this could be generally useful I
went ahead and implemented support for the metadata that is found on some,
not all, platforms.

TBD is providing better support for interpretating the `mode` value.

Related: elves#1659
krader1961 added a commit to krader1961/elvish that referenced this issue Apr 4, 2023
I originally started work on this to provide a mechanism to verify the
behavior of other unit tests; e.g., a `path:chmod` command I'm working on.
Primarily because the external `stat` command has different option names
depending on whether you're on a BSD or Linux platform making it hard to
use portably. However, since it seems like this could be generally useful I
went ahead and implemented support for the metadata that is found on some,
not all, platforms.

TBD is providing better support for interpretating the `mode` value.

Related: elves#1659
krader1961 added a commit to krader1961/elvish that referenced this issue Apr 13, 2023
In theory the `path:stat` command introduced by this change allows an
Elvish implementation of something like the Unix `ls`, or Windows `dir`,
command. More importantly, it 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 related commands because
they are probably used often enough it doesn't make sense to deprecate them.

Related: elves#1659
@krader1961
Copy link
Contributor Author

This comment by @xiaq on my change to implement a path:remove has caused me to think more about this issue. There are reasonable arguments for and against deviating from the Go packaging of this functionality in Elvish. After sleeping on the question I am inclined to agree that OS specific functionality, such as removing files or creating directories, should live in an Elvish os module. There is a good argument that such functionality applies to file paths and therefore should reside in the Elvish path module. But on balance I agree with @xiaq that such functionality is fundamentally tied to interacting with the OS rather than manipulating filesystem path names (such as by concatenating path name components).

TBD is what to do about the existing path:is-dir and path:is-regular commands. Arguably they should be deprecated and moved to an os module. And if they aren't deprecated and moved what does that mean for new commands such as os:stat (aka path:stat) I want to see implemented?

krader1961 added a commit to krader1961/elvish that referenced this issue Apr 21, 2023
I considered adhering more closely to the traditional Unix `rm` and
`rmdir` commands. For example, by implementing each independently and
supporting the `--force` option. However, I concluded there are
insufficient reasons to do so. In particular, the POSIX `--force` (or
`-f`) option is most often used solely for its side-effect of making
elimination of a non-existent path a non-error. I also don't like that
`-f` also fixes permission errors to allow allow removing files. If we
decide that feature is needed it should be added under a distinct option
(e.g. `&fix-perms`).

Related: elves#1659
xiaq added a commit that referenced this issue May 8, 2023
This addresses #1659.
krader1961 added a commit to krader1961/elvish that referenced this issue Aug 5, 2023
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 added a commit that referenced this issue Aug 21, 2023
Time fields are omitted for now; they will be added when there's a proper Elvish
binding for time.Time.

This addresses #1659.
krader1961 added a commit to krader1961/elvish that referenced this issue Oct 24, 2023
krader1961 added a commit to krader1961/elvish that referenced this issue Oct 26, 2023
krader1961 added a commit to krader1961/elvish that referenced this issue Oct 26, 2023
krader1961 added a commit to krader1961/elvish that referenced this issue Oct 26, 2023
xiaq added a commit that referenced this issue Feb 19, 2024
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

No branches or pull requests

1 participant