-
Notifications
You must be signed in to change notification settings - Fork 209
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
Export the Reporter API #123
Conversation
e3557d2
to
9a8bdee
Compare
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.
Neat.
cmp/report.go
Outdated
func (r *defaultReporter) Report(f reportFlags) { | ||
if f&reportUnequal > 0 { | ||
func (r *defaultReporter) Report(f ReportFlags) { | ||
if f&ReportUnequal > 0 { |
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.
Optional. I didn't call this out on previous CL, but I tend to view single bit mask comparison as != 0 or == 0. > 0 seems to imply more values.
cmp/options.go
Outdated
|
||
// ReportEqual reports whether the node is equal. | ||
// This may be ORed with ReportByMethod or ReportByFunc. | ||
ReportEqual |
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 this ever not be ORed with anything? If so, it'll be pretty easy to misuse.
Instead of exposing the implementation detail of ReportFlags
being a bitset in the API, how about giving it Equal
, Unequal
, Ignored
, ByMethod
, and ByFunc
methods instead?
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.
The downside to individual methods is that we cant add new ones to the reporter interface. For example, when we add cycle-detection support (#85), we would probably want to add ReportByCycle
to indicate that the comparison was determined because of a detected cycle.
What are your thoughts on an opaque bit-set type?
type ReportFlags struct {
_ [0]func() // Make ReportFlags incomparable
flags uint
}
func (f ReportFlags) Has(f2 ReportFlags) bool {
return f&f2 != 0
}
var (
ReportEqual = ReportFlags{flags: 1}
ReportUnequal = ReportFlags{flags: 2}
ReportIgnored = ReportFlags{flags: 4}
ReportByMethod = ReportFlags{flags: 8}
ReportByFunc = ReportFlags{flags: 16}
)
This way, it is impossible to accidentally do ==
on the flags. The only operation that you can do is call the Has
method.
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.
The downside is that ReportEqual
is a variable instead of a constant. In theory someone could over-write it, but thats true for any sentinel variable like io.EOF
.
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'm suggesting methods on the ReportFlags
type (and perhaps rename it to Report
in case something non-flag is added to it in the future):
func (Reporter) Report(r Report) {
if !r.Equal() {
// ...
}
}
Having both Equal
and Unequal
methods looks a bit strange, so perhaps either define Equal
as returning true for ignored values or make Equal/Unequal/Ignored an enum:
if r.Type() == cmp.ReportEqual {
}
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 like the suggestion. Take a look.
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 like it.
The Reporter option allows users to hook in their own custom reporters to programatically interpret the diff structure. The Reporter API uses a push/pop mechanism, which is strictly more powerful than an API that only calls report on leaf nodes. The Reporter.Report method takes in a Result type to provide flexibility in what properties can be reported in the future since new properties can be added, but new methods cannot be easily added to the reporter interface.
The Reporter option allows users to hook in their own custom reporters
to programatically interpret the diff structure.
The Reporter API uses a push/pop mechanism, which is strictly more powerful
than an API that only calls report on leaf nodes.
The Reporter.Report method takes in a flag set to provide flexibility in
what properties can be reported in the future since new constants can be added,
but new methods cannot be easily added to the reporter interface.