Skip to content

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

Closed
tophtucker wants to merge 1 commit into
mainfrom
toph/more-warnings
Closed

Auto: error on reducer without input; warn on ordinal size; expose warnings#1424
tophtucker wants to merge 1 commit into
mainfrom
toph/more-warnings

Conversation

@tophtucker

@tophtucker tophtucker commented Apr 4, 2023

Copy link
Copy Markdown
Member

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

Fil commented Apr 4, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member 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
Copy Markdown
Member 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