lppedd / idea-conventional-commit

Context and template-based completion for conventional/semantic commits.
https://plugins.jetbrains.com/plugin/13389-conventional-commit
MIT License
336 stars 18 forks source link

Add better support for footer issue references #43

Open mavolin opened 3 years ago

mavolin commented 3 years ago

Currently, when autocompleting a footer token, a colon is always added. While this is the desired behavior in most cases, the conventional commit specification states that this should not be done, if followed by an issue reference, e.g. Closes #123:

Each footer MUST consist of a word token, followed by either a :<space> or <space># separator, followed by a string value (this is inspired by the git trailer convention).

~ Conventional Commit Spec, Point 8

Possible Implementation

First of all, when linting the commit message, a warning should be shown, if the footer's value starts with ": #". This could be paired with the option to auto-fix this, depending on whether the footer's value is an issue reference or plain text, to remove the colon or the '#`, respectively.

Additionally, an "issue reference" option could be added to the footer types settings. This should be turned on for Closes, Implements, and Refs. If turned on, "#" will be inserted after the token, instead of ":". This should cover most cases, as issue refs are mostly used for those three.

lppedd commented 3 years ago

Thanks for the feedback!

This is something I thought about - I think - in March/April but considered as a minor issue and never solved.
You're right about the colon, it should be omitted in case a reference starting with a hash is written down.
The problem is I can't, obviously, predict what a user is going to write after the footer type and so I decided to stick with it always.

As a starting point what I will do is improve the Conventional Commit inspection and its attached fix.

Following I will have to decide something regarding how to auto-insert a colon or a hash based on context.
I don't really like the "issue reference option" you proposed because it's "heavy" (toggling it requires opening the Settings panel).

So, what about auto-removing the colon from the footer type as soon as you type the hash in the footer value? Do you think it's a valid alternative?

Edit: I will go back to the option switch if I find impractical intercepting the hash. I need to test it first.

mavolin commented 3 years ago

I actually like that idea even better, but am fine with either if this turns out to be too much effort. One thing to keep in mind when doing this is that the colon should probably not be replaced if it was manually typed, as there could be cases where one wants to start a text with an hash for whatever reason.

lppedd commented 3 years ago

@mavolin

where one wants to start a text

Let's say someone picks Close, the output is:

Close:<caret>

Then the user types another colon

Close::<caret>

And then a hash

Close::#<caret> // or Close: :#<caret>

Here I will not touch the text. Is this what you meant?

lppedd commented 3 years ago

Ah ok, you updated the message. Heh, that's a problem. I don't think I can actually understand when the colon is typed manually. I mean probably it's possible, but it would mess up the code. I need to check.

mavolin commented 3 years ago

It's not that important, so I guess it'll work just fine without.

mavolin commented 3 years ago

Starting a sentence with a hash is pretty rare anyway.

lppedd commented 3 years ago

@mavolin my conclusion is that the best way to do this is not to catch "manual" insertion of the colon.
Instead I'm going to provide an option - Auto-remove footer type's colon if value is a reference - enabled by default.

A user is then able to opt-out, which will be rare.
I will also need to update the footer RegExp.

lppedd commented 3 years ago

This issue requires rewriting the footer RegExp. I don't know if I can make it for the next week. Is this urgent?

mavolin commented 3 years ago

No, not at all. Take any time you need.

lppedd commented 3 years ago

@mavolin I'm really sorry for long wait. I had busy weeks prior to Christmas :(

mavolin commented 3 years ago

No worries, this isn't urgent or anything of the sort. Do it whenever you have time.

lppedd commented 2 years ago

The lexer now works correctly with both cases. This issue will be closed when a completion contributor based on the PSI structure is implemented, potentially in 0.22.0.