-
-
Notifications
You must be signed in to change notification settings - Fork 163
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
Better stringify list error #1764
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for trying it, and improving the error! Minor suggestions, I would like something short and concise, but still having all the info
osh/word_eval.py
Outdated
@@ -232,6 +232,11 @@ def _ValueToPartValue(val, quoted, part_loc): | |||
s = val_ops.Stringify(val, loc.Missing) | |||
return part_value.String(s, quoted, not quoted) | |||
|
|||
elif case(value_e.List): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On line 231 above, I guess we could add value_e.List` so Stringify is called, and then we can reuse the same error message?
It's a little subtle but we could add a comment to call that out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk if the comment is OK as it is. 😅
ysh/val_ops.py
Outdated
@@ -120,6 +120,11 @@ def Stringify(val, blame_loc, prefix=''): | |||
val = cast(value.Eggex, UP_val) | |||
s = regex_translate.AsPosixEre(val) # lazily converts to ERE | |||
|
|||
elif case(value_e.List): | |||
raise error.TypeErrVerbose( | |||
"%sexpected Null, Bool, Int, Float, Eggex, got List. Use '@' instead of '$' to splice the returned List into Strings" % prefix, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about something shorter, which still conveys the point (users tend not to read long messages):
- "Use @ instead of $ to splice" ?
- "Use @ instead of $ to splice a List"
- "List can't be substituted. Use @ instead of $ to splice"
I kind of like the last one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah my first idea is very long actually! I was also thinking about displaying a simple help command to show more infos. I just checked if there is any reasonable doc page to reference, but I couldn't find one. Is there some (reference) docs about switching between command & expression mode? Then we could have a message like
Use @ instead of $ to splice or see "help switch-modes"
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried another error message now and removed the long expected .......
list, not sure if it is better. 😄 FWIW I think stringified
sounds a bit clearer than substituted
(to me, at least). But it's definitively not "good" yet.
What do you think of the other suggestion of something like join()
?
%got a List, which can't be Stringified! Use @ instead of $ or join()"
ysh/val_ops.py
Outdated
@@ -120,6 +120,11 @@ def Stringify(val, blame_loc, prefix=''): | |||
val = cast(value.Eggex, UP_val) | |||
s = regex_translate.AsPosixEre(val) # lazily converts to ERE | |||
|
|||
elif case(value_e.List): | |||
raise error.TypeErrVerbose( | |||
"%got a List, which can't be Stringified! Use @ instead of $ or join()" % prefix, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo -- should be %s and not %g
I would say
"Maybe use @ to splice, instead of $"
The problem is that this error can actually appear in many places
We have noticed in the past that overly specific error messages can become misleading -- i.e. trying to be helpful doesn't always work:
It would also help to add another function to test/ysh-runtime-errors.sh
, with all the examples that are intended to be changed by this commit
Then we can run it and see how they look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a possibility to check the AST if we're at a $
?
The "dream" scenario IMO would be something like:
ysh ysh-0.19.0$ write $[:|a b c|] write $[:|a b c|]
^~ Use @ instead of $ to slice
[ interactive ]:1: fatal: expected Null, Bool, Int, Float, Eggex, got List. Can't stringify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible in theory ... we have all the info necessary
But I don't think we have any examples of it. We would have to enumerate all the cases and make sure that the error makes sense.
I think this actually DOES exist in test/ysh-runtime-errors.sh
, but I suppose it's not organized well enough to actually find them! I guess we could grep the output and find them.
Whenever I write error messages, I put examples in that file, and eyeball them to see if they make sense.
As mentioned it's easy to construct an error for a specific situations, and then find it doesn't apply to other situations that hit the same code path
I think it's OK to say "Maybe" or "Hint" in those cases
BTW I think this is a good change, I would just like some demos in We can add more precision in later PRs, but I think it's good to do a first pass of polishing here |
I fetched out all functions into a file called
Seems like there is currently no case where A list is substituted into a word :( I'll add some cases in there but I really gotta do some work now 😅 |
Ha this is a good way to do it ! No rush, whenever you have time |
🤦🏼 I searched for the wrong string… It's what happens when I do all this stuff sometime in the night when my daughter needs to be in the baby carrier to sleep - heavily sleep deprived 😅 anyway I found exactly 2 errors for which the message is perfect:
(and copy paste keeps messing with spaces/newlines, it makes me mad!) but it should probably be:
|
I think this message is pretty good, except
could be
to "parse" better (though I prefer shorter error messages in general, this seems clearer) Or how about
I don't think we can really know what the "right" fix is. We just know that @ goes with List and $ goes with String I would lean toward leaving out the
|
@andychu just saw that this is still open. I did push a commit with your recommended changes. Do you think this is fine to merge or something left to do? |
Ah sorry I didn't realize you pushed another change! In general please write a message here when it's ready for review, since sometimes people push and they don't want a review yet Looks good, merging. Thanks! |
Yeah fair. I actually often let a PR be in inconsistent state for some time! Thanks for merging :) |
Working on "immediate help-on-error" in #1824 I noticed your discussion here. I think something that worked out good is a bit of separation between the error message + context and the helping hints. For the message here it might look like:
Maye have a newline before and after the bullet list? |
Describe a way to fix the error when accidentally using
$
instead of@
.Since this is something that will probably happen to a lot of
bash
users, it probably makes sense to be a bit helpful here :)and