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

Tests and tweak for Interval component #436

Merged
merged 10 commits into from Jan 17, 2019

Conversation

Projects
None yet
3 participants
@philip-peterson
Copy link
Contributor

commented Jan 16, 2019

Hi! Thank you for making Dash, it is a very helpful platform for quickly creating visualizations. I was using the Interval component the other day and found out that there is some trouble with the disable prop (I made an example to show how). So I added some tests and made some tweaks to the Interval component, hopefully the fix is useful to you!

Cheers,

  • p
@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

Thanks @philip-peterson! The fix looks good, and quite a bit clearer code too 🎉

I linted and rebuilt (we're intending to change the process so built files are published only with releases, but for now we're building in every PR). That said, eslint is still complaining about the console.warn on a redundant startTimer call. I think we have that rule in place because console messages are not a level of debugging we want to persist in the final code - either a) this is a severe enough problem that it should throw an actual error, b) it's not an error at all and we should just return silently, or c) we should treat this as equivalent to resetTimer, and clear and restart the interval. I don't think (a) makes sense, probably I'd go for (b) but could be convinced for (c).

FYI I should just note that events are about to disappear (see plotly/dash#531) but I'll take care of reconciling that with the work and tests in this PR after merging.

@philip-peterson

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Hey Alex, thanks for taking a look at this so quickly! I thought about it a bit, and actually I think option (A) is what would make the most sense. Since we are talking about a situation that should never occur -- the code is written in a way that should not allow it to happen -- we would want to know if something has gone wrong, because it means something in the code isn't written the way it was intended. That eliminates option B. As for option C, if someone is managing to go down this path, it is by accident (either on their part or ours), and the last thing a developer wants alerting them of a mistake that they made is the presence of some subtle, slightly different behavior from what constitutes normal operation. So that would rule out option (C).

The argument against option (A) I can see as being that, well, if the component is broken then we should at least try to proceed with execution. But, I would counter that attempting to proceed with executing code known to be broken is doomed to fail.

Let me know what you think of all that. Pushed up a couple new commits to fix that lint issue mentioned and also ran prettier on all the tests. Thanks again!

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

OK, as long as we have sufficient test coverage to be comfortable that this error is never thrown, which it looks like we do, I can support throwing an error.

@Marc-Andre-Rivet this is a 💃 from my standpoint, any comments before I merge it? (I added a changelog entry just now FYI)

@philip-peterson philip-peterson referenced this pull request Jan 16, 2019

Closed

disabled prop for dcc.Interval is not used. #325

0 of 4 tasks complete
@Marc-Andre-Rivet

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

💃. Thanks for reaching out to us with these fixes and improvements @philip-peterson!

@alexcjohnson alexcjohnson merged commit b5bf870 into plotly:master Jan 17, 2019

4 checks passed

ci/circleci: python-2.7 Your tests passed on CircleCI!
Details
ci/circleci: python-3.6 Your tests passed on CircleCI!
Details
ci/circleci: python-3.7 Your tests passed on CircleCI!
Details
percy/dash-core-components Visual review automatically approved, no visual changes found.
Details
@philip-peterson

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

Thanks @Marc-Andre-Rivet and @alexcjohnson ! This ticket can be marked as solved btw: #325

@philip-peterson philip-peterson deleted the philip-peterson:interval-tests branch Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.