numberscope / frontscope

Numberscope's front end and user interface: responsible for specifying sequences and defining and displaying visualizers
MIT License
7 stars 15 forks source link

fix[Formula]: reject unknown function at parse to avoid throwing error #498

Closed gwhitney closed 1 week ago

gwhitney commented 1 week ago

By submitting this PR, I am indicating to the Numberscope maintainers that I have read and understood the contributing guidelines and that this PR follows those guidelines to the best of my knowledge. I have also read the pull request checklist and followed the instructions therein.


Resolves #427.

gwhitney commented 1 week ago

Sorry about the stacked PRs, but this one is straightforward and orthogonal to #497. Feel free to respond to/review either in whatever order is convenient. Whichever gets merged first, rebasing the other will be trivial.

katestange commented 1 week ago

This behaves as described and fixes that problem. But I got a popup that permutations requires second argument to be less than first argument. There are still going to be tons of popups as we use this. Is it possible to route all popups that derive from mathjs to a red error message like this PR does for unknown function?

gwhitney commented 1 week ago

Well the halting problem says we can't detect all error-throwing at parse time. So the best I can do is put a catch on the computation and post the error message; it may be murky though. I will implement something and you can give it a try.

katestange commented 1 week ago

Well the halting problem says we can't detect all error-throwing at parse time. So the best I can do is put a catch on the computation and post the error message; it may be murky though. I will implement something and you can give it a try.

Yeah, I guess I was hoping just that pop-ups (that require a click) would show up as red messages instead (no click), if we can tell they are coming from the user typing in the sequence box. I don't know how much can be done in this direction, but if there's a way to tell they are because of typing in the box, we could just re-route from pop-up to red error. We might occasionally get weird error messages there I suppose.... I look forward to seeing what you try.

gwhitney commented 1 week ago

OK, give that a try. Let me know what you think.

katestange commented 1 week ago

So far I really like this. I haven't looked at the code yet, because I wanted to just mess around and discover what it does. I haven't been able to prompt a pop-up even doing weird stuff like making matrices and whatnot. And the limit (1022 additional warnings discarded) is really great! :)

katestange commented 1 week ago

Ok I did manage to provoke some pop-ups. The main thing is the following, which happens when I use functions that don't ouput the right type to interpret as a Bigint for a sequence. A simple way to provoke this is to type typeOf(n) in the formula box. Screenshot from 2024-11-16 16-55-22

gwhitney commented 1 week ago

OK, now it checks for NaN inside the catch block, so it is even more robust. What do you think now?

katestange commented 1 week ago

OK, now it checks for NaN inside the catch block, so it is even more robust. What do you think now?

Why does it give this seemingly nonsensical error, though? Screenshot from 2024-11-16 17-32-16

gwhitney commented 1 week ago

Umm because the value returned by the formula typeOf(n) is the string "number", which indeed cannot be converted to a number. Would you like me to include the name of the data type in the error message?

katestange commented 1 week ago

Umm because the value returned by the formula typeOf(n) is the string "number", which indeed cannot be converted to a number. Would you like me to include the name of the data type in the error message?

Oh my gosh that totally confused me! :) Yeah, if you are able to easily include "of type ..." that might help someone clueless like me. :)

gwhitney commented 1 week ago

How's that read?