-
Notifications
You must be signed in to change notification settings - Fork 44
Conversation
does not crash if user restores default value and sets it to empty string ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but as this is something we will add to other places, we may want to tackle the awkward "null"
notation used for indicating a default value. See comment inline.
autonat.go
Outdated
@@ -77,5 +77,5 @@ type AutoNATThrottleConfig struct { | |||
// global/peer dialback limits. | |||
// | |||
// When unset, this defaults to 1 minute. | |||
Interval Duration `json:",omitempty"` | |||
Interval Duration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small UX issue after testing it in go-ipfs's optional-duration branch
Setting works as expected via ipfs config AutoNAT.Throttle.Interval 1s
, but to unset I had to execute ipfs config AutoNAT.Throttle.Interval null
which produced an awkward JSON with "null"
(a string), not an actual null
(without quotes):
"AutoNAT": {
"Throttle": {
"GlobalLimit": 0,
"Interval": "null",
"PeerLimit": 0
}
},
Not the best way of indicating "default" value.
I've added more tests and support for ""
(empty) string in c9b7984 so users don't break their nodes when they remove custom value by hand, but when done via CLI getting the "null" is awkward.
Is there a way to get ""
or null
or "default"
instead of "null"
?
The "default"
is the most user-friendly and would be preferable in my mind.
I'll look into this and push more tests. We should be liberal with values that we interpret as "use default" + serialize "default" to something more intuitive than "null" (with quotes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the logic for marshaling the default in MarshalText
:
func (d Duration) MarshalText() ([]byte, error) {
if d.value != nil {
return []byte(d.value.String()), nil
}
return []byte("default"), nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps switching to (Un)MarshalJSON
rather than text would help you out here.
@Stebalien any context for why you went with Text rather than JSON originally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched to *JSON in 16af6f8 – *Text was probably used to avoid handling double quotes, but now that we want explicit string as the default (instead of null
) *JSON is easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, the mentioned change removed some paper cuts.
Took it for a spin and now, no matter how user tries to restore default and ruin their day..
$ ipfs config AutoNAT.Throttle.Interval ""
$ ipfs config AutoNAT.Throttle.Interval null
$ ipfs config --json AutoNAT.Throttle.Interval '"null"'
$ ipfs config AutoNAT.Throttle.Interval default
..for all the above (done via CLI or a manual edit of JSON config) the Duration will correctly serialize to:
"AutoNAT": {
"Throttle": {
"Interval": "default",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh.. so I've played with this a bit while testing Swarm.RelayService
config from ipfs/kubo#8522 and we will have Field: null
all over the place due to OptionalInteger
and Flag
anyway.
We can't have "default" string for those, so I think its better to unify and switch Duration to use null
as well. At least it will be consistent and easy to document.
It is also what OptionalString from #149 will do.
The default will now look like this:
"AutoNAT": {
"Throttle": {
"Interval": null,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With recently added UX improvements described in #148 (comment) this LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched defaults to use JSON null
(matching existing behavior in OptionalInteger, Flag and other types)
848e479 made it even smarter, when used as *OptionalDuration
json:",omitempty"
works again.
This makes it possible to use OptionalDuration with `json:",omitempty"` so the null is not serialized to JSON, and get working WithDefault as well.
* feat: make it possible to define optional durations * test: empty/default optional durations does not crash if user restores default value and sets it to empty string "" * refactor: use null in JSON * refactor(duration): use JSON null as the default Rationale: ipfs/go-ipfs-config#148 (comment) * refactor: Duration → OptionalDuration This makes it possible to use OptionalDuration with `json:",omitempty"` so the null is not serialized to JSON, and get working WithDefault as well. Co-authored-by: Marcin Rataj <lidel@lidel.org>
* feat: make it possible to define optional durations * test: empty/default optional durations does not crash if user restores default value and sets it to empty string "" * refactor: use null in JSON * refactor(duration): use JSON null as the default Rationale: ipfs/go-ipfs-config#148 (comment) * refactor: Duration → OptionalDuration This makes it possible to use OptionalDuration with `json:",omitempty"` so the null is not serialized to JSON, and get working WithDefault as well. Co-authored-by: Marcin Rataj <lidel@lidel.org>
* feat: make it possible to define optional durations * test: empty/default optional durations does not crash if user restores default value and sets it to empty string "" * refactor: use null in JSON * refactor(duration): use JSON null as the default Rationale: ipfs/go-ipfs-config#148 (comment) * refactor: Duration → OptionalDuration This makes it possible to use OptionalDuration with `json:",omitempty"` so the null is not serialized to JSON, and get working WithDefault as well. Co-authored-by: Marcin Rataj <lidel@lidel.org>
* feat: make it possible to define optional durations * test: empty/default optional durations does not crash if user restores default value and sets it to empty string "" * refactor: use null in JSON * refactor(duration): use JSON null as the default Rationale: ipfs/go-ipfs-config#148 (comment) * refactor: Duration → OptionalDuration This makes it possible to use OptionalDuration with `json:",omitempty"` so the null is not serialized to JSON, and get working WithDefault as well. Co-authored-by: Marcin Rataj <lidel@lidel.org>
This is needed for #146, so we can deal with optional durations the same way we deal with optional integers.
Disadvantage:
omitempty
doesn't work any more. Instead, we'll have anull
in the JSON represenation.The
Duration
is only used in theAutoNATThrottleConfig
, so this is minimal change. This go-ipfs branch is the only change that's required.