Fixed whitespace issue when generating match

[vallentin]

Fixes #397

• Fixed the generator to emit whitespace correctly
• Changed generator to not emit inter whitespace between match and the initial when
  • Additionally, updated the parser and removed inter from Node::Match
• Updated existing test cases
• Added additional match test cases, to check all the variants of suppressing whitespace

[vallentin]

From this point forward, I'll probably create a branch within the main repo instead of in my fork.

[vallentin]

Since this is a non-breaking change and just a bug fix. I assume it's alright that I just squash and merge it.

[djc]

No, it's really not... I thought I mentioned on Twitter that I would still review your proposed changes. (I also mentioned some best practices for commits which you didn't follow here.) I've changed the repository settings to require approval in a PR before merging (including for my own PRs).

[vallentin]

I would still review your proposed changes

Alright, sorry. I won't merge again. :)

I also mentioned some best practices for commits which you didn't follow here

I'm not entirely sure I understand. Maybe I misunderstood something, but I thought you meant you wanted 1 commit on the main branch per PR. So I thought any amount of commits in a PR would be alright, given that it gets squashed anyways.

[djc]

I'm not entirely sure I understand. Maybe I misunderstood something, but I thought you meant you wanted 1 commit on the main branch per PR. So I thought any amount of commits in a PR would be alright, given that it gets squashed anyways.

No, I want clean linearized commits on the main branch (so I usually aim for "Rebase and merge" on PRs), each of which on its own can pass the tests/rustfmt/clippy, but each of the commits should also be minimal. In this case, I'd have liked to have the first commit (which needs a better commit message) separate from the second commit, but the third commit should be folded into the first (otherwise the tests don't pass on the first commit). The fourth and fifth commit logically belong together, and the sixth commit should also be folded in there (otherwise rustfmt doesn't pass on the fourth + fifth commit).

Does that make some sense? I usually use interactive rebasing to make sure I get a clean commit history, I'm happy to help guide you through that if you don't have experience with it yet.

[djc]

(And BTW, it's not about merging per se -- I'm fine if you merge things after you have approval + passing CI.)

[vallentin]

each of the commits should also be minimal

This is why I split the first and second commit. So it would be easier to revert and undo the second commit, in case you wanted to keep the parser as is. Though I do now see that yeah, the first commit would not pass CI. I was thinking of everything as a whole, in relation to everything becoming a single commit in the end. But I do understand what you mean now.

first commit (which needs a better commit message)

What do you prefer? e.g. "Fixed whitespace issue when generating match" or "Fixed whitespace issue when generating match (#397)". Personally, I'd prefer "Fixed #397", thus why I'm asking :)

But I do overall understand what you meant now. Not to repeat myself, but I was honestly just thinking in terms of everything being squashed into a single commit. So I didn't really think much about breaking CI in between.

I usually use interactive rebasing to make sure I get a clean commit history

I've never rebased a branch that was an active PR. But I'm assuming that if I rebase and squash commits, that GitHub will act correctly.

[djc]

This is why I split the first and second commit.

Yeah, and that part is much appreciated!

Fixed whitespace issue when generating match (#397)

I'd probably go for "Fix whitespace suppression in match expressions (fixes #397)", but your second variant is also much better. The first line of the commit message should be enough for me to recall some context (just the issue number doesn't do that), although I do try to also have the issue number in there so I can look up more context.

I've never rebased a branch that was an active PR. But I'm assuming that if I rebase and squash commits, that GitHub will act correctly.

You'll have to push with --force/-f, otherwise Git won't like it, but that's fine.