Skip to content

Conversation

@domoritz
Copy link
Contributor

fixes #209

@mbostock
Copy link
Member

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

@jheer
Copy link
Contributor

jheer commented May 20, 2020

import {halts} from ‘halting-problem’;

Or instead perhaps a reasonable maxiter value?

@domoritz
Copy link
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
Copy link
Member

mbostock commented May 20, 2020

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

@domoritz
Copy link
Contributor Author

domoritz commented May 20, 2020

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

@jheer
Copy link
Contributor

jheer commented May 20, 2020

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

@domoritz
Copy link
Contributor Author

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

@domoritz
Copy link
Contributor Author

@mbostock could you include this patch in a release?

Copy link
Contributor

@curran curran left a comment

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.

@domoritz
Copy link
Contributor Author

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

@curran
Copy link
Contributor

curran commented Jun 15, 2020

@domoritz
Copy link
Contributor Author

I sent #212

@domoritz
Copy link
Contributor Author

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

Copy link
Member

@Fil Fil left a comment

Choose a reason for hiding this comment

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

LGTM

@domoritz
Copy link
Contributor Author

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

@domoritz
Copy link
Contributor Author

ping @curran @Fil @mbostock

@Fil
Copy link
Member

Fil commented Jul 24, 2020

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

@domoritz
Copy link
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