talonvoice / talon

Issue Tracker for the main Talon app
85 stars 0 forks source link

Feature Request: surface user talon code/grammar errors more aggressively and with more context #126

Open rntz opened 4 years ago

rntz commented 4 years ago

Errors in user python code get reported quite helpfully: they appear in the talon log marked with ERROR and a trace back, and also cause a notification. This makes it very easy to notice that you've screwed up your python code. Errors in TalonScript user code are harder to notice. I'm thinking in particular of parse errors and uses of incorrectly named lists and captures. These do show up in the talon log, where they look like this:

2020-09-28 22:05:52    IO Rule not found: user.symbol
2020-09-28 22:05:52    IO List not found: user.code_libraries
2020-09-28 22:07:05    IO ParseError: did not understand definition on line 1: 'fubar ba'

But these lines are marked with IO, so they aren't visually distinct from more innocuous lines, and they do not cause notifications, which makes it harder to notice them; and [*] they don't come with a trace back or other indication of where the error occurred, which makes it harder to debug them.

It would be nice if these were also marked with ERROR or WARNING, or at least something visually distinct from IO, and if the "rule not found" and "list not found" errors indicated the code responsible in some way.

[*] Except for the ParseError, which comes with a line number - you can also infer the file name from the DEBUG line above it indicating the file that was loaded. So that's all right.

rntz commented 4 years ago

Motivating code example for "Rule not found" error

Suppose that keys.py previously defined captures named <user.symbol> and <user.number> but they've been recently renamed <user.symbol_key>, <user.number_key>. However, formatters.py has a line that wasn't properly updated:

formatters.py

# NB. uses <user.symbol>, <user.number>
@ctx.capture(rule="(<user.symbol> | <user.number>)")
def formatter_immune(m): ...omitted...

This will produce the following at various points (it seems to happen whenever the DFA rules get built? and I'm not sure why user.number doesn't also get a rule not found error):

2020-09-28 22:55:38    IO COMPILING
2020-09-28 22:55:38    IO dfa rules built in 0.069s
2020-09-28 22:55:38    IO Rule not found: user.symbol

If this error message is overlooked, and if <formatter_immune> is used in a place where arbitrary text could be used instead (eg. see example below), the result is annoying to debug, because exactly the same words will be matched but the behavior will be different than expected.

@ctx.capture(rule="<self.formatters> <user.text> (<user.text> | <user.formatter_immune>)*")
def format_text(m): ...omitted...

I've used this example because it's how this problem actually came up, if you'd prefer a minimized example with no parts omitted I can make one.

rntz commented 4 years ago

I'm still trying to track down what's causing this "List not found: user.code_libraries" error, as it looks like user.code_libraries is actually properly declared. But in principle at least the same kind of thing could happen with lists instead of captures.

lunixbochs commented 4 years ago

Is it declared, but no context has initialized it to at least a blank list?

rntz commented 4 years ago

Indeed. Is the right thing to do to define it to be an empty list in an empty context?

lunixbochs commented 4 years ago

Yes, the list doesn't exist in the speech engine unless the value has been set somewhere.

lunixbochs commented 3 years ago

I think "define it to be an empty list in an empty context" won't be necessary in the next beta. I made an attempt to make module declared lists default to "empty list".

rntz commented 3 years ago

Another case where more aggressive errors/warnings would be helpful: Suppose that I declare a list tex_greek and later assign to the list tex_greek without the required user. or self. prefix. No warning is produced; the assignment is silently ignored.

tex.py

mod.list("tex_greek", desc="...")
ctx.lists["tex_greek"] = { "alpha": "alpha" }
# ^ this is a bug, and should be ctx.lists["self.tex_greek"].

tex.talon

# does not match any spoken inputs because user.tex_greek is empty
greek {user.tex_greek}: ...
lunixbochs commented 3 years ago

Are you sure it never errors? Even if you restart talon it doesn’t error on startup?

rntz commented 3 years ago

huh, that's confusing. I swear it wasn't producing a warning, but I tried it again and it's producing a warning in the log:

2020-12-01 00:53:21 WARNING lists: skipped because they have no matching declaration: (tex_greek)

I didn't even have to restart talon (for clarity, it's likely that I restarted talon between my previous post and this one, but I didn't have to restart talon after re-introducing the bug to get the warning to occur). Sorry for the false (probably?) alarm.

lunixbochs commented 3 years ago

Is it possibly this https://github.com/talonvoice/beta/issues/118

rntz commented 3 years ago

Don't think so, pretty sure I wasn't updating any lists from actions anywhere. I'll keep an eye out if I see it again but my guess for now is I just didn't check the log and somehow thought I had.