Skip to content

Ensure that niced domains always span ticks#210

Merged
Fil merged 3 commits into
d3:masterfrom
domoritz:fix-nice
Jul 30, 2020
Merged

Ensure that niced domains always span ticks#210
Fil merged 3 commits into
d3:masterfrom
domoritz:fix-nice

Conversation

@domoritz

Copy link
Copy Markdown
Contributor

fixes #209

@mbostock

Copy link
Copy Markdown
Member

Now the question is there any situation we aren’t anticipating that will go into an infinite loop. :)

@jheer

jheer commented May 20, 2020

Copy link
Copy Markdown
Contributor

import {halts} from ‘halting-problem’;

Or instead perhaps a reasonable maxiter value?

@domoritz

Copy link
Copy Markdown
Contributor Author

We could attempt to prove that this specific code will always halt but better be safe. I added a commit to limit the number of iterations with maxiter as @jheer suggested.

@mbostock

mbostock commented May 20, 2020

Copy link
Copy Markdown
Member

I think my preference is to prove that it will always halt, but that requires more mental effort. :)

@domoritz

domoritz commented May 20, 2020

Copy link
Copy Markdown
Contributor Author

Ideally, we would prove it for all future versions of the code as well.

@jheer

jheer commented May 20, 2020

Copy link
Copy Markdown
Contributor

Everyone please be considerate. The maxiter suggestion is designed to make sure eslint feels consulted and content.

@domoritz

Copy link
Copy Markdown
Contributor Author

@mbostock I'm happy with the code. Is there anything you want me to change?

@domoritz

Copy link
Copy Markdown
Contributor Author

@mbostock could you include this patch in a release?

@curran curran left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The logic appears sound. The incidental var --> const/let changes will make the codebase internally inconsistent. Maybe use var everywhere? I'm all for ES6 migration, but maybe that would be best to do on all modules at once, as a separate PR.

Comment thread src/linear.js Outdated
@domoritz

Copy link
Copy Markdown
Contributor Author

I went back to var and will send a separate pull request switching to let/const.

@curran

curran commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

@domoritz

Copy link
Copy Markdown
Contributor Author

I sent #212

@domoritz

Copy link
Copy Markdown
Contributor Author

Anything else you want me to change @mbostock @curran @Fil?

@Fil Fil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread src/linear.js
@domoritz

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews @curran and @Fil. @mbostock can you include this fix in a release?

@domoritz

Copy link
Copy Markdown
Contributor Author

ping @curran @Fil @mbostock

@Fil

Fil commented Jul 24, 2020

Copy link
Copy Markdown
Member

yes I think so—we're currently preparing a rather big release (ie d3v6)

@domoritz

Copy link
Copy Markdown
Contributor Author

🎉 Awesome. Looking forward to it.

@Fil Fil merged commit 0a55cc8 into d3:master Jul 30, 2020
@domoritz domoritz deleted the fix-nice branch July 30, 2020 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Ticks of niced linear scales do not always cover the domain

5 participants