Closed krader1961 closed 6 months ago
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.
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.
Related: #1659