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

include scale name in error #1298

Merged
merged 1 commit into from
Feb 28, 2023
Merged

include scale name in error #1298

merged 1 commit into from
Feb 28, 2023

Conversation

mbostock
Copy link
Member

Includes the scale name in errors for easier debugging.

I also rewrote the isOrdered helper to iterate directly, instead of materializing all the pairs using d3.pairs and then testing with array.every. I think it’s cleaner this way, and slightly faster, not that it matters.

@mbostock mbostock requested a review from tophtucker February 28, 2023 19:35
@mbostock mbostock force-pushed the mbostock/better-error branch from 0d2ba4c to 850a3fa Compare February 28, 2023 19:37
Copy link
Contributor

@tophtucker tophtucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice! Better!

(isMonotonic in auto.js could probably be rewritten as isOrdered(domain, orderof(domain)) if we want one fewer helper?)

@mbostock
Copy link
Member Author

(isMonotonic in auto.js could probably be rewritten as isOrdered(domain, orderof(domain)) if we want one fewer helper?)

I thought about that, and no, the behavior is actually slightly different. The one for the threshold scale is more strict because it doesn’t allow non-comparable values either. At any rate I don’t think it’s worth the effort to DRY yet.

@mbostock mbostock merged commit 8d2d1b3 into main Feb 28, 2023
@mbostock mbostock deleted the mbostock/better-error branch February 28, 2023 20:25
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
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.

2 participants