Templates without `<% return ... %>` in `successCommentCondition`/`failCommentCondition` have confusing behavior

[jedwards1211]

I can't see any reason to use Lodash templates other than <% ... %> eval expressions for successCommentCondition/failCommentCondition. I think we can make it more elegant. Should we automatically wrap with <%/%> so that the user-supplied string can just contain the logic?

[babblebey]

Hmmm, sounds fair... But I believe in acknowledging the usage of lodash template and letting users freely write their own evaluation in the technology too;

Automatically wrapping it in the <% ... %> seem kinda cool, but it would also mean that we are taking the usage of lodash template to the background and will have to provide support for that part.

I'd like to hear what @travi think about this 🤔

[jedwards1211]

Actually a template like ${ issue.pull_request } on issue: { pull_request: false } would evaluate to the string 'false' which is considered truthy by the existing logic, defying what the user would have intended. So templates string interpolation instead of <% ... %> won't even work right now.

> _.template('${ issue.pull_request }')({issue: {pull_request: false}})
'false'

[jedwards1211]

Hearts sure is breaking the comment formatting

[babblebey]

Hearts sure is breaking the comment formatting

Ooops, yea.. That should be fixed by now 😉

[travi]

sorry for the slow response here. i'm not sure i have a strong opinion on this one. i've never used the templating functionality. simplifying is attractive from a maintenance perspective, but i'm not sure i have much of a grasp of what other folks might be using this functionality for.

if we do remove it, it should be considered breaking

[jedwards1211]

We can at least consider this a bug right?

a template like '${ issue.pull_request }' on issue: { pull_request: false } would evaluate to the string 'false' which is considered truthy by the existing logic

[travi]

sounds like it. do you think the presence of that bug suggests lack of use by consumers? seems like that would suggest that maybe no one is using it anyway?

[jedwards1211]

Well, looks like this feature is only 8 months old, and the readme only provides examples using <% return ... %> so probably no one has tried anything else. I just realized the bug because I find the syntax for these conditions distasteful so I started thinking about what concrete problems it might cause.