Fix for #1025

[pepkin88]

Fix for #1025.

This fixes placeholder handling in match and with calling logic operators, although the latter doesn't need placeholders at all.

I still think this calling logic operators feature is a miss. In terms of usability and intuition, I strongly prefer match over this, even though match isn't even documented.

[rhendric]

Thanks for contributing! Is there a reason this added check only applies to the _ clause? It seems to me like it should guard test being assigned or something.

Also, please squash this down to a single commit.

[rhendric]

Also also, there should be no change to parser.js; I assume the reason you have a diff there is because you're running Node.js <10? If you can't/don't wish to upgrade to Node.js 10, just don't git add that file to the commit.

[pepkin88]

From what I understand, the | test instanceof Var and test.value is \_ line was there to detect the use of a placeholder _, as _ outside of match is a variable. The problem was, this test was too broad and caught not only _, but also _.isString (and calls too, like _!, as I just noticed, I'll add a test for that in a moment). So I narrowed the test by checking if the chain doesn't have any tails (like in the case of _.isString). The other tests don't need such narrowing. That is what you are asking, correct?

Regarding squashing, isn't it done while merging, as they said there? Other than that, I don't quite know how to do that, I don't understand why those Merge remote-tracking branch 'gkz/master's pop up there. I'm using GitKraken UI for this and it looks like it's ok, but I'm really not sure, I'm not a git expert.

Regarding the changes in parser.js, that's correct, I haven't set up Node.js 10 yet, I'll upgrade it.

[rhendric]

Everything you've observed is correct, but I think you've only gotten 90% of the way to the root of the problem. The entire auto-compare method only examines the head of the chain, and if a head matching a special case is found, the existence of any tails is ignored (and any such tails, if present, are discarded). You've fixed this problem for only one case in that method; consider what might happen with, for example,

match option
| 'widgets'starts-with => widgets
| 'gadgets'starts-with => gadgets

I think the better fix would be to examine @head only if @tails is empty, like test = @head unless @tails.length, thus sending all tail-having chains to the otherwise clause. What do you think?

Re Git strategy, it's true that I could squash the commits myself (through GitHub or through Git), but it's nicer if contributions come organized in the commit(s) they'll have in the repository (that way, the original author can write the commit messages and the maintainer doesn't have to rewrite anything). While there's no universally agreed upon standard for commit organization, the standard I favor is one commit per feature or fix, which usually means one commit per pull request but can sometimes mean a few if there are multiple, interrelated features being developed and reviewed together. I don't know much about Git GUIs, but the easiest way to squash down your commits from the command line is git rebase -i origin/master, and change all commits but the first from pick to squash (or s). Then git push -f to update the pull request (pull requests are an exception to the rule not to force-push to remote repositories).

[pepkin88]

I think I know now what I do wrong with those commits, the next PR has only one. I'll try to follow your steps, but isn't it hard to change commits that are already on the server? I've never made it through. So if I fail then this one will have to be squashed while merging to master, sorry about that.

[pepkin88]

Oh, I got you now, you are right. Good test case.

[pepkin88]

I think it's good now :) Take a look

[rhendric]

Great, thank you!