-
Notifications
You must be signed in to change notification settings - Fork 204
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
vg/recorder: invalid/unused Canvas.c field #771
Comments
On Fri, 2023-09-01 at 06:30 -0700, Sebastien Binet wrote:
I was in the middle of implementing a JSON-based vg/recorder.Canvas
(b/c of go-hep/hep#982), looking at the code in vg/recorder.
when I noticed:
func (c *Canvas) append(a Action) {
if c.c != nil {
a.ApplyTo(c)
}
c.Actions = append(c.Actions, a)
}
I think a.ApplyTo(c) should read a.ApplyTo(c.c).
right ?
but Canvas.c is never accessed (outside of Canvas.append, that is),
and cannot be modified outside of vg/recorder.
so, couldn't we just get rid of this field ?
I think you are correct on both of these. I don't remember what I was
thinking when that was written. My suspicion is that I was thinking
about replay, but it's not required for that either.
Dan
|
sbinet
added a commit
to sbinet-gonum/plot
that referenced
this issue
Sep 2, 2023
Fixes gonum#771. Signed-off-by: Sebastien Binet <binet@cern.ch>
sbinet
added a commit
that referenced
this issue
Sep 2, 2023
Fixes #771. Signed-off-by: Sebastien Binet <binet@cern.ch>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I was in the middle of implementing a JSON-based
vg/recorder.Canvas
(b/c of go-hep/hep#982), looking at the code invg/recorder
.when I noticed:
I think
a.ApplyTo(c)
should reada.ApplyTo(c.c)
.right ?
but
Canvas.c
is never accessed (outside ofCanvas.append
, that is), and cannot be modified outside ofvg/recorder
.so, couldn't we just get rid of this field ?
The text was updated successfully, but these errors were encountered: