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.

[LuisUrrutia]

successCommentCondition/failCommentCondition should always finish in a boolean value, which means it's safe to asume that we could parse an string type return into boolean.

function conditionParser(returned) {
	if (isString(returned) && returned.toLowerCase() === 'false') {
		return false;
	}

	return Boolean(returned);
}

This example fn is just a reference to help me explain the idea.

I know Boolean([]) and Boolean({}) would return true. But right now those are evaluated to true
The question is, it is safe to assume that 'false' it's going to be false? From my perspective, it is.

Otherwise, I would keep it simple, and fail if the return of applying template it's something different than a boolean.

Either way, I agree that "fixing" this issue would result in a breaking change.