feat(git): cherry-pick keyword supports tag attribute

[elliot-nelson]

📑 Summary

Cherry-picked commits are commits as well, and might be tagged (especially if you are cherry-picking commits into a release branch from a trunk branch). So it makes sense to support the tag attribute in the cherry-pick keyword.

Resolves #

📏 Design Decisions

For syntax, the most straightforward implementation is to copy from the commit keyword.

For display, we are currently already rendering the tag cherry-pick:<ID> for a cherry-pick. Here, I think the most flexible change is to allow the tag attribute to override this default tag and completely replace it, which allows the user to choose to extend it (e.g. cherry-pick:A snapshot) or to use alternate text (e.g. bug fixed).

📋 Tasks

Make sure you

• 📖 have read the contribution guidelines
• 💻 have added unit/e2e tests (if appropriate)
• 🔖 targeted develop branch

[aloisklink]

Looks great! Thanks for adding docs and a test case.

Two issues:

• Currently there's a bug in cherry-pick, however, that's not related to your PR, I've got a fix in Fix broken cherry-pick parsing in gitgraph #3480
• The .jison code has the wrong arguments. See my comment on how to fix.

After applying #3480 and my suggestion, the e2e tests render correctly:

[elliot-nelson]

Thanks @aloisklink! I'll admit I was scratching my head a bit at the relationship between jison and the renderer, now it makes more sense.

I did also make an additional commit which I'm open to feedback on... it would be nice if explicitly writing tag:"" did what the user would expect (hide the tag), rather than defaulting to the default tag. This is the classic "what string represents default?" -- I tried to choose one that straddles the line between "unlikely for users to run into" and "understandable to future code readers".

[aloisklink]

Commit 4e4b5cc throws an error, since you're using sourceCommit at the beginning of the function, but it's only declared later on in the function.

My gut feeling is to use the JavaScript standard way of doing things for default parameters (feel free to disagree if you or another reviewer feels otherwise). If tag === null || tag === undefined, then we can use the default variable. There's a newish feature in JavaScript called the nullish coalescing operator (??) that makes this easy to implement (see my comment for more details).

Edit:

Thanks @aloisklink! I'll admit I was scratching my head a bit at the relationship between jison and the renderer, now it makes more sense.

Hahaha, yeah, I also barely understand how the jison stuff works :) It's old legacy code we'd love to replace.

By the way, it might help to add parsing tests to your code in src/diagrams/git/gitGraphParserV2.spec.js. It'll be a bit difficult since the cherry-pick command is currently broken (my fault), but if you run git pull https://github.com/aloisklink/mermaid.git fix/broken-cherry-pick-parsing you should be able to pull my changes from PR #3480 into your branch.

[elliot-nelson]

Makes sense! I wasn't sure if jison params had to be strings, if not then undefined makes perfect sense.

I'll make the tweaks you suggested later today.

[elliot-nelson]

Well, I think I've got it working, but can't successfully run e2e still...

I've tried with:

start-server-and-test dev http://localhost:9000/ 'cypress run --spec "**/git*"'  

But the resulting git graph diagrams in cypress/snapshots still seem to show the Syntax error PNG.

[aloisklink]

Well, I think I've got it working, but can't successfully run e2e still...

I see the issue, it looks like the current jison rules don't actually allow an "". There needs to be at least one character for jison to return a string. In the following lines, a "" will call this.begin("string"); and this.popState(); without ever calling return STR;.

mermaid/src/diagrams/git/parser/gitGraph.jison

Lines 60 to 62 in e8eb2ab

	["] this.begin("string");
	<string>["] this.popState();
	<string>[^"]* return 'STR';

I've got no idea how to fix this (or if it's even possible to fix this) since I'm not familiar with jison. Maybe another @mermaid-js reviewer will know?

Maybe instead of using tag: "", we could use something like tag: NONE or no-tag?

If it's too much work, you're welcome to also move the allow cherry-pick to suppress tag altogether commit into another PR. It'll be nice to be able to disable tagging, but your PR is already really useful even without that feature!

[elliot-nelson]

Aha, I didn't realize quite how these screenshots got saved (looks like successes go in snapshots/ and failures go in screenshots/).

To help troubleshoot I took your earlier advice and added a couple tests to the git parser spec, and I think my suggested fix to the JISON will work.

I could not figure out a generic way for STR to support empty strings, but maybe that's OK... that might be actually be a regression since a lot of things that would be syntax errors today could end up being unexpected program errors instead. With a dedicated EMPTYSTR token, you have to write twice as many lines if empty strings are OK, but at least it won't accidentally regress anything.

[aloisklink]

dedicated EMPTYSTR token

Perfect! That works great, since using cherry-pick ... tag:"" is probably the best syntax to use for disabling a tag. That's a smart way to use jison.

It all looks good for me (thanks for adding parsing tests too!). Now we just need to wait for other maintainers to review!

[ashishjain0512]

Thanks for your contribution. This will be a definite add-on!