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.

Sign up for GitHub

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

Jump to bottom

coerce ccw to true or false to avoid inversions #22

Closed
wants to merge 2 commits into from
Closed

coerce ccw to true or false to avoid inversions #22

wants to merge 2 commits into from

Conversation

Fil
Copy link
Member

@Fil Fil commented Jun 6, 2019

(fixes #21)

@Fil Fil requested a review from mbostock June 6, 2019 20:42
@mbostock
Copy link
Member

mbostock commented Jun 7, 2019

I think !!ccw is more standard than !!+ccw. Any reason not to use the former?

@Fil
Copy link
Member Author

Fil commented Jun 7, 2019

It was for the case of "0".

  • !! makes "0" true-ish
  • !!+ makes "0" false-ish

"0" is currently triggering the bug, which means we're sure that no-one uses it, and either solution is fine. Removing the + will save one byte :)

@mbostock
Copy link
Member

mbostock commented Jun 7, 2019

But "0" is a truthy value, and thus should be interpreted as true rather than false.

if ("0") {
  console.log("it’s true");
}

@mbostock mbostock closed this Aug 1, 2019
@mbostock mbostock deleted the trueish-ccw branch August 1, 2019 16:59
@mbostock
Copy link
Member

mbostock commented Aug 1, 2019

Rebased and merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbostock mbostock mbostock approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

path.arc(…anticlockwise) must be true or 1, not truthy
2 participants
@Fil @mbostock