Skip to content
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

Fix bug on rule.remove #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix bug on rule.remove #40

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 29, 2012

context.rule creates a line with the datum {id: id}.

In the rule focus handler, this whole datum is set to an integer i, and the id is lost.

rule.remove will then throw an error because it tries to access d.id (with d being the datum) but the datum is just an integer now and has no 'id' property.

The fix consists of simply adding the integer i as an additional property of the datum instead of completely replacing it.

context.rule creates a line with the datum {id: id}.

In the rule focus handler, this whole datum is set to an integer i, and the id is lost.

rule.remove will then throw an error because it tries to access d.id (with d being the datum) but the datum is just an integer now and has no 'id' property.

The fix consists of simply adding the integer i as an additional property of the datum instead of completely replacing it.
@RandomEtc
Copy link
Collaborator

Thanks for this pull request; apologies for the delayed response.

I think I understand what's going on and this sounds like the right fix. I don't fully understand the way that things with class line are used, I see there are also metric line objects which have .data(values) - do these need an ID as well? In your fork they are here.

Can you help me merge this by:

  1. signing our individual contributor license agreement
  2. making the change to the corresponding file in cubism's src folder?
  3. dig into the way metric line classed things are used and make sure this fix doesn't break those?

Thanks again!

@jra3
Copy link

jra3 commented Apr 4, 2013

🚢

@RandomEtc
Copy link
Collaborator

Thanks for the boat of confidence, @jra3, but this pull request isn't quite ready. Can you help with items 2 or 3 on my checklist?

@jra3
Copy link

jra3 commented Apr 4, 2013

I'll give it a shot. Ran across this pull request while investigating the bug myself.

@RandomEtc
Copy link
Collaborator

Awesome - thanks!

You'll need a different pull request from your fork, just reference this one and we'll pick up the discussion there. We'll need you to fill out the contributor agreement too, it's just a quick Google Doc to make a couple of things explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants