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:chmod #1730

Closed
wants to merge 1 commit into from
Closed

Implement os:chmod #1730

wants to merge 1 commit into from

Conversation

krader1961
Copy link
Contributor

Related: #1659

@krader1961
Copy link
Contributor Author

There are other uses of generic errors that, arguably, should use the errs.Generic this change introduces. Such as ttySetupErr in TestReadCode_AbortsWhenTTYSetupReturnsError (pkg/cli/app_test.go). I considered those cases too far outside the scope of this change.

@xiaq
Copy link
Member

xiaq commented Jan 3, 2024

Thanks for the contribution. My thoughts:

  • I don't like the introduction of errs.Generic; it already has two different use cases in this PR - in test code where the identity of the error matters, and in non-test code that only cares about the presence of the error. The former should continue to use a unique error value (so that it can't accidentally match other places that use errs.Generic), and the latter should be refactored to use a boolean instead to keep track of whether the conversion succeeded.

    In general, duplication is preferable than ambiguous abstraction.

  • The os:stat command already has a convention for representing non-permission non-type bits using a separate special-modes list. This API is easier to use than the traditional bit flag API, at least from Elvish code (Elvish currently lacks bitops, which makes this worse although that should be fixed regardless). It is less efficient than bit flags, which is a trade-off that makes sense for Elvish.

    The os:chmod command should use that pattern too: the signature should be fn chmod {|&special-modes=[] perm file| }.

  • I was a bit ambivalent about whether os:stat should return the permission bits as a typed number or a string containing an octal, but ultimately I felt that as a programming language the former was the only sensible choice, so I'd rather os:chmod be consistent in this regard.

    One thing I'll introduce later is the ability for numbers to have a default format. The perm bits in the output of os:stat can then be displayed as base-8 by default (but still with the 0o prefix so that there is no ambiguity). This is only relevant for converting a number to a string, and not relevant for the conversion in the other direction though - so os:chmod should still accept numbers like any other builtin.

Thanks again for the contribution but I won't be merging this PR; I have prepared my implementation that I will push shortly.

@xiaq xiaq closed this Jan 3, 2024
xiaq added a commit that referenced this pull request Jan 3, 2024
This supersedes #1730.
@krader1961 krader1961 deleted the os-chmod branch January 27, 2024 07:04
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