This repository was archived by the owner on Mar 9, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 44
feat: add an OptionalDuration type #148
Merged
Merged
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
d62365e
feat: make it possible to define optional durations
marten-seemann c9b7984
test: empty/default optional durations
lidel 16af6f8
refactor: use null in JSON
lidel 753de75
refactor(duration): use JSON null as the default
lidel 848e479
refactor: Duration → OptionalDuration
lidel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 executeipfs config AutoNAT.Throttle.Interval null
which produced an awkward JSON with"null"
(a string), not an actualnull
(without quotes):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
""
ornull
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
: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..
..for all the above (done via CLI or a manual edit of JSON config) the Duration will correctly serialize to:
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 haveField: null
all over the place due toOptionalInteger
andFlag
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: