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

Auto: error on reducer without input; warn on ordinal size; expose warnings #1424

Closed
wants to merge 1 commit into from

Conversation

tophtucker
Copy link
Contributor

@tophtucker tophtucker commented Apr 4, 2023

Demo. Sketching some additional errors / warnings based on what I've seen leading people astray.

Error on reducer without input — Setting a non-count reducer without an input field error, not warns, because it's a static error in the config, regardless of what data gets passed in. TODO: do any other reducers not need an input field?

Warn on ordinal size — Since this is dynamic, it’s just a warning, not an error. One issue is there’s no way to suppress it if for some reason you want this. It should probably be subsumed by #493, but I was just itching to try something.

Expose warnings (#1192) — In this PR, my approach is to set an array of warnings on the returned figure. Mike has mentioned he might prefer passing in an onWarn callback. I guess the advantage there is that these warnings could be triggered by interaction in the future? Right now, the chart cell almost never runs into warnings, but there are situations where it seems like it should (like ordinal size), so I feel the itch to be able to expose the warnings in its UI.

@tophtucker tophtucker mentioned this pull request Apr 4, 2023
13 tasks
@Fil
Copy link
Contributor

Fil commented Apr 4, 2023

sum does not need an input (it defaults to count); distinct works too I think, also giving count. min-index and max-index will always give 0. And then there are the x1, x2 reducers (for binX), which give the bins' bounds.

@tophtucker
Copy link
Contributor Author

Even though sum and distinct work as count, I've seen several people get confused by selecting "distinct" in the chart cell and seeing nothing change, so I'm still sort of inclined to throw (or warn?) on them.

@tophtucker
Copy link
Contributor Author

I broke out the one solid piece (error on underspecified reduce) to #1833.

#493 and #1192 remain as open issues; I don’t feel good enough about the approaches in this PR.

@tophtucker tophtucker closed this Aug 24, 2023
@tophtucker tophtucker deleted the toph/more-warnings branch August 24, 2023 15:26
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