search

Chia-Network / clvm

[Contract Language|Chia Lisp] Virtual Machine
Apache License 2.0
86 stars 35 forks source link

unify pattern for error messages #48

Closed arvidn closed 3 years ago

arvidn commented 3 years ago

when passing a list where an atom is expected Consistently print all arguments passed to the operator in the error message

richardkiss commented 3 years ago

At the risk of splitting hairs off a bikeshed, doesn't it seem to make more sense on XXX on list to print just the single argument that caused the problem, rather than enumerating all arguments? There are arguments each way. Pro: it's more clear which argument causes the problem. Con: it might be more clear (especially if there are more than one argument) which invocation of the operator in your program caused the problem. What do you think @aqk @arvidn ?

arvidn commented 3 years ago

I'm in favor of just printing the argument that caused the problem. From what I can tell, we have two forms of this error:

  1. "XXX requires int args"
  2. "XXX on list"

They are basically the same kind of error and initially I considered unifying them as well, but I thought maybe that would be going too far.

In (1), we always print all the args (except "softfork"), in some cases in (2) we print just the non-atom argument. This PR is the smallest change to unify it, but going the other way would simplify the code further, as far as I can tell.

arvidn commented 3 years ago

I've updated (force pushed) this patch to only print the argument that failed the check (i.e. wasn't an atom). The patch is larger but I think it's better.

arvidn commented 3 years ago

updated. I also made sure to add unit tests for these cases that weren't covered

arvidn commented 3 years ago

and rebased

aqk commented 3 years ago

About printing the arguments: I mainly use the error message to locate the expression with the error, more so than figuring out what the error was, so if e.g. (i 0 4) gives "too few args", it's a bit nicer to have the full expression.

The fanciest is to print the sub-expression that caused the issue, and highlight the problem, but we don't need that right now.

Either way is good for now.