JS: introduce 'Util::describeExpression'#390
Conversation
ghost
added
the
ghost
self-requested a review
as a
| if sel instanceof VarAccess then | ||
| msg = capitalize(description) + " always evaluates to " + cv + " here." | ||
| else | ||
| msg = "This " + description + " always evaluates to " + cv + "." |
There was a problem hiding this comment.
In general we could get messages like:
This property 'f' always evaluates to true.
In general I think "named" expressions should belong in the former category.
Property 'f' always evaluates to true here.
We could add a boolean flag to describeExpression indicating whether it is named.
There was a problem hiding this comment.
Hmm, on the other hand, if we decided to name calls, then they would go in the other category,
This call to `isFoo` always evaluates to true.
Call to `isFoo` always evaluates to true here.
Natural langauge sucks.
There was a problem hiding this comment.
The problem is the two kinds of messages. How about we conflate them to the latter, with variables being described as "read of variable 'v'":
Examples:
This comparison always evaluate to true.
This expression always evaluate to true.
This read of variable 'v' always evaluate to true.
This read of property 'p' always evaluate to true.
This call to 'f' always evaluate to true.
This property read always evaluate to true.
This call always evaluate to true.
There was a problem hiding this comment.
I would prefer This use of instead of This read of.
There was a problem hiding this comment.
Done + some additional features. I am not sure I like messages with the callee name though: they are long, and the callee name is not a link.
|
Rebased. |
3 participants
As discussed recently, it may be clearer to mention the kind of an expression in an alert message, instead of just using the generic "this expression is bad".
This PR introduce utility for doing just that:
Util::describeExpression. I have put it to use injs/trivial-conditional, we can use it in other queries later.The evalution shows that performance does not degrade.
Ping @max as he may have an opinion on this.