fix: String method on the OptionalString #153
Conversation
types.go
Outdated
| @@ -358,9 +358,9 @@ func (p *OptionalString) UnmarshalJSON(input []byte) error { | |||
|
|
|||
| func (p OptionalString) String() string { | |||
| if p.value == nil { | |||
| return "default" | |||
| return "" | |||
There was a problem hiding this comment.
Current convention is that String() methods return default to state that the value is default:
Line 104 in eb0d046
Line 203 in eb0d046
Line 104 in eb0d046
I think we shoulkd switch it back and to do the same here. Better to be explicit and consistent here.
( alternative was to change it everywhere to something more explicit like <default> )
| return "" | |
| return "default" |
If you want to get "" on the consumer side of things, you want to use WithDefault("") – see
https://github.com/ipfs/go-ipfs/blob/228570814df94ab7bdd9052a4c705c1bf4c7f0b9/core/node/groups.go#L321
There was a problem hiding this comment.
Switched to "default" in 5861089
lidel
force-pushed
the
fix-optionalstring-string
branch
from
e1a9f97
to
eb0d046
Compare
There was a problem hiding this comment.
Merging for the sake of merging useful fixes and new tests, thanks!
(I don't feel strongly about specifics of the default String(), only want things to be consistent across types, but don't want that discussion to slow down release. We can tackle it in a separate issue or a PR, if you feel strongly either way.)
lidel
changed the title
+ test fixes