cmp: add Or #60204
Comments
|
Duplicate of #14423 |
|
@seankhliao, good find, but that issue was frozen before the modern proposal process existed. Either that issue should be reopened and put through the proposal process or this one should be unclosed, but I don't think it's fair to call it a duplicate when the old one was never actually evaluated. |
|
The idea was clearly evaluated in the previous issue and declined. |
|
I don't think it's fair to call 5 comments "clearly evaluated." The reception was mixed. Abbgrade was for it. Minux was against it. Bradfitz was neutral to positive on the idea if there was more data. It ends with @griesemer saying,
I don't think he would have said "go discuss it somewhere else" if that discussion was precluded from having an effect because the issue was permanently closed once and for all. I think the idea was "go discuss it more and if it comes up again we can take another look." Now we have a formal process, so it's time to take a look. :-) |
|
It was quite clear it doesn't belong in strings, and the natural place for it be in now, slices, also has the similar idea being declined in #52006 |
|
It doesn't work in slices because it would need a T comparable, which is confusing, or to be a find func, which as you note was already declined. Just because it could be a generic doesn't mean it should be. :-) I've been using my personal stringutils.First for years and for me it's above the bar to get it into the standard library. Maybe I'm wrong, but I think it's worth having a discussion. |
|
I agree that the earlier issue didn't get a full proposal review. We can do it again. That said, finding some more examples would help justify adding this. And, in general the strings and bytes package are parallel. What would this look like in bytes, and would anybody use that variant? |
I've been using a version of First for at least three years that I can recall, and I'm up to 19 uses in a 13,000 line project. It's pretty routinely useful for me. (It might go back further, and I've just forgotten the history of it.) Going back to the examples from archive/tar above, I think there's a readability gain in
I suppose it should be |
|
I’m doing some very basic searching on SourceGraph to find versions of this in the wild.
Okay, that’s as much looking at search results as I feel like doing now. If anyone can do a more semantic search over a larger corpus, I would be interested to see the results. One thing that surprised me was how often a repo would have multiple versions of it. Also the env var default thing comes up a lot. Edit: Couldn't help myself, and I found another one in Istio 😆 Gotta force myself to close the tab before I go crazy. |
|
That's great data, thanks. |
|
This can be written for any comparable type using generics today func First[T comparable](vs ...T) T {
var zero T
for _, v := range vs {
if v != zero {
return v
}
}
return zero
}The type constraint could be loosened to While often for strings, I've written similar for all kinds of types, though I don't think I've ever needed anything other than 2 values at a time. It's quite common in dealing with configuration where the zero value models an absence to be replaced by a default. |
|
I’ve had a toy repo with generic First for several years, but I’ve found that in practice I only ever use strings. As for varadic vs a pair, most instances are just pairs, but I think the Go optimizer now optimizes the slice away, so you may as well have a variadic version for the occasional times when you need more than two. |
Neither |
|
This is clearly a useful operation, perhaps even useful enough to have in the standard library. But is First the right name? Is it the name used anywhere else with this meaning? In text/template (and also in Lisp and Scheme, where I took it from), the name for this operation is |
|
It's also similar to the min/max builtins proposed in #59488 except that the item is selected in a less mathematical and more Go-specific way |
|
Even if it's mostly used for strings, it really feels not string-related to me and not a good fit for the However, I think I would use it quite a bit for not-strings if it existed. There's a certain kind of operation I write regularly in Go which would be written using a ternary in another language. Something like (this is grabbed from some real code): With a ternary expression, you might write something like With a slices-based function, you could do I think A longer, but more self-evident name is |
|
This proposal has been added to the active column of the proposals project |
That's #37165, which also uses a default port as an example. :-) |
|
I'm fine with the name |
|
I don't think this is necessarily a great idea, but just to consider it, you could have package bools with |
I guess the follow-up is, what package would it fit in? Looking through #60204 (comment) I think it is pretty surprising that this is so frequently about environment variables (or at least configuration variables with similar usage patterns). I wonder if there might be a more Glasgow (not New Jersey)-style package for aggregating configuration from program constants, env variables, json/yaml/toml etc., and I think this functionality would probably be natural in that style. OTOH, applying the New Jersey philosophy, maybe it's not objectionable that people are re-implementing this functionality ad-hoc - it doesn't seem error-prone, and people can be as narrow or as abstract as they want. |
|
One place where I've written a lot of code like this: func New(cfg *Cfg) *Thing {
t := &Thing{
foo: cfg.Foo
}
if t.foo == nil {
t.foo == fooDefault
}
return t
}If there were an func New(cfg *Cfg) *Thing {
return &Thing{
foo: or(cfg.Foo, fooDefault),
}
} |
|
Reading through the comments in #60933, I’m concerned that despite finding a way to prevent implicitly converting to a non zero interface we still won’t have a way to make something like cmp.Or(os.Stdout, io.Discard) work correctly. Even with explicit types, it seems busted. It makes me wonder if we really want cmp.Or to be generic or if it might be safer to go with something concretely typed. |
|
type appEnv struct {
out io.Writer
}
…
if verboseFlag {
app.out = os.Stdout
}
…
_, err := io.Copy(cmp.Or(app.out, io.Discard), logSrc) |
|
Ok, but I don’t think that addresses the issue. If you use a field with an actually nillable *os.File, cmp.Or fails again. |
|
@hherman1 There is a long-standing tripping point in Go regarding storing a typed |
|
No change in consensus, so accepted. 🎉 |
|
Recently, I encountered a piece of code where I thought a logical XOR operation could be helpful. if found && wanted || !found && !wanted {
// Nothing to do.
return
}With the introduction of the package cmp, I imagined that there might be an Xor function: if cmp.Xor(found, !wanted) {
// Nothing to do.
return
}However, I recalled that
Since I just wanted to bring it up for discussion. |
|
I actually had truthy.Xor in an old version of my truthiness testing package, but I removed it because I wasn’t actually using it. |
|
I salute Carl's effort to collect usage data. However, I remain unconvinced that such a functionality warrants an addition to the standard library. Projects that need such an The semantics are different, but I would keep this function out of the standard library for the same reason I wouldn't want to see the addition of a |
Sorry for commenting very late this issue. I did not react to this proposal because emoji-meter (👍👎) in the description suggested no acceptance. I understand the use cases found in the collected data. Thanks for finding and summarize them 🙏🏼 My concern is readability and I do not know if Can this example also be written like this? type appEnv struct {
out io.Writer
}
…
if verboseFlag {
app.out = os.Stdout // logic is only here (1)
} else {
app.out = os.Discard // and here (2) and written only with existing if and else
}
…
_, err := io.Copy(app.out, logSrc)If the previous example can also be written this way, then there is a good example of why this example could fragment the writing of logic and ultimately make it difficult to read because of inconsistency. There are now multiple ways to write this code. However, if this is very rarely the case, then please ignore my comment. In the future, should you read The implications for consistent writeability are unclear to me, and I hope this is considered in this proposal. |
|
The app.out example seems to me more like it should be written with hypothetical type appEnv struct {
out io.Writer
}
…
app.out = Cond(verboseFlag, os.Stdout, io.Discard)
…
_, err := io.Copy(app.out, logSrc)Cond would be basically the ?: ternary but as a function and without short circuiting. I agree that having Cond would lead to changes in the Go ecosystem that would affect the experience of reading it. I don't think it's a good fit for Go as it exists. The most clear straightforward to write the code ought to be something like: type appEnv struct {
out io.Writer
}
…
app.out = io.Discard
if verboseFlag {
app.out = os.Stdout
}
…
_, err := io.Copy(app.out, logSrc)Yes, you could use cmp.Or here, but that seems like more work and less clarity than just using an assignment, so I don't think it will come up too much in practice. We'll see! |
New code constructions will be introduced slowly and their use will be determined by the developer who does not have so much clarity. If But on the other hand I have no concrete examples and if it remains only with the use as with the code examples found thanks to carlmjohnson then |
Fixes golang#60204 Change-Id: I1234cacf0f25097d034038bcfb33f6630373a057 GitHub-Last-Rev: e9098ed GitHub-Pull-Request: golang#60931 Reviewed-on: https://go-review.googlesource.com/c/go/+/504883 Auto-Submit: Ian Lance Taylor <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Than McIntosh <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> Reviewed-by: qiulaidongfeng <[email protected]>
and others
An extremely common string operation is testing if a string is blank and if so replacing it with a default value. I propose adding
First(...strings) stringto package strings (and probably an equivalent to bytes for parity, although it is less useful).Here are three example simplifications from just archive/tar because it shows up first alphabetically when I searched the standard library:
archive/tar diff
The text was updated successfully, but these errors were encountered: