-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't omit history index when the value is 0 #1545
Conversation
Pull Request Checklist
|
Jenkins Builds
|
services/browsers/database.go
Outdated
@@ -35,7 +35,7 @@ type Browser struct { | |||
Name string `json:"name"` | |||
Timestamp uint64 `json:"timestamp"` | |||
Dapp bool `json:"dapp?"` | |||
HistoryIndex int `json:"history-index,omitempty"` | |||
HistoryIndex *int `json:"history-index,omitempty"` |
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.
Wouldn't it be enough to remove omitempty
? Or we still need to distinguish between 0
and no history index?
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 think it is correct to distinguish between 0 and no history. history index should be consistent with the history array. e.g. if there is no array or no items in array - history index 0 is confusing
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.
In theory, no history
might be considered that there is no history instead of looking at the index. I guess that would be the same? Just *int
looks odd in this case but all is correct :)
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 guess it makes sense, a consumer will have to double check that index is not out of bounds anyway
closes: #1544