Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow long URLs in commit messages #59

Closed
johnboyes opened this issue Aug 9, 2020 · 8 comments · Fixed by #61
Closed

Allow long URLs in commit messages #59

johnboyes opened this issue Aug 9, 2020 · 8 comments · Fixed by #61

Comments

@johnboyes
Copy link

johnboyes commented Aug 9, 2020

I propose that URLs should always be allowed in the body of commit messages, even when longer than 72 characters.

Given that commit messages should be well-documented, relevant URLs should be encouraged, I'd say. Alternatives such as using a URL shortener have downsides (makes the writing of the commit message more cumbersome, and makes the actual destination URL opaque).

There seems to be something of a consensus in the open source world around allowing long URLs (1, 2, 3, 4).

If you agree and decide to add this feature, I'd propose as a potential implementation skipping the line length check for any line that has a URL in it. I think a check that only allows long URLs if they are the only content on a given line would be too restrictive, as a common approach is to have the urls sequentially numbered as reference links at the foot of the commit message.

@mristin
Copy link
Owner

mristin commented Aug 9, 2020

Thanks for the suggestion! Let me also check what markdown proposes.

Give me a couple of days as it's a busy summer time at the moment.

@johnboyes
Copy link
Author

johnboyes commented Aug 10, 2020

Have been thinking about this a little more. Given that this GitHub Action is opinionated (which is a great thing), one option would be to allow (and encourage) reference links, but not inline links, thoughts?

Personally I think that could be a very nice approach, as reference links are much more elegant than inline links when reading commit messages IMHO, and it would enforce one clear consistent way of specifying links.

Would also make sense to allow lines which just have a url and nothing else (i.e. the url is not a reference or inline link but rather is just shown in the message as a url)

Just sharing my thoughts :-)

mristin added a commit that referenced this issue Aug 14, 2020
This patch allows long URLs on separate lines or as link definitions
in the body. This is necessary so that the link functionality does not
break in the readers such as terminals and Github.

Fixes #59.
mristin added a commit that referenced this issue Aug 14, 2020
This patch allows long URLs on separate lines or as link definitions
in the body. This is necessary so that the link functionality does not
break in the readers such as terminals and Github.

Fixes #59.
mristin added a commit that referenced this issue Aug 14, 2020
This patch allows long URLs on separate lines or as link definitions
in the body. This is necessary so that the link functionality does not
break in the readers such as terminals and Github.

Fixes #59.
@mristin
Copy link
Owner

mristin commented Aug 14, 2020

Hi @johnboyes ,
I finally implemented both versions as people probably use variety of URL styles and both are acceptable. For example, in-line links can still be readable:

[See this page](
http://some-domain.com/long/long/long/long/path.html
).

and sometimes reading the URL helps the understanding (e.g., StackOverflow questions). If referenced, the reader looses the extra information up to the point that s/he reads the URL in the link definition.

I released a new patch version including this feature (2.1.1, https://github.com/mristin/opinionated-commit-message/releases/tag/v2.1.1).

Thanks again for the suggestion!

johnboyes added a commit to SpectoLabs/hoverfly-github-action that referenced this issue Aug 14, 2020
Updated to the latest version of the commit message checker which
contains [this new feature][1]

Closes #15

[1]: mristin/opinionated-commit-message#59
johnboyes added a commit to SpectoLabs/hoverfly-github-action that referenced this issue Aug 14, 2020
Updated to the latest version of the commit message checker which
contains [this new feature][1]

Closes #15

[1]: mristin/opinionated-commit-message#59
@johnboyes
Copy link
Author

johnboyes commented Aug 14, 2020

Thanks v much for implementing this so quickly, @mristin 😎

I've just given it a quick test and I'm still getting the long line error after upgrading to v2.1.1 - v grateful if you can take a look pls?

Here is the commit message which is failing:

Ensure long urls in commit messages are allowed

Please see [this page for more details][1] or read the manual.

[1]: http://some-domain.com/very/long/long/long/long/long/long/long/long/long/path.html

@mristin
Copy link
Owner

mristin commented Aug 14, 2020

Hi @johnboyes ,
This is a strange bug -- I explicitly test for that case. I'll try to have a look at it during the next week.

@mristin mristin reopened this Aug 14, 2020
@mristin
Copy link
Owner

mristin commented Aug 15, 2020

Hi @johnboyes ,
It seems I simply forgot to include dist folder in the commit. Could you please check if the new patch version (2.1.2) works for you?

@johnboyes
Copy link
Author

Just tested again and works perfectly now, thanks @mristin 👍

@mristin mristin closed this as completed Aug 15, 2020
johnboyes added a commit to SpectoLabs/hoverfly-github-action that referenced this issue Aug 15, 2020
Updated to the latest version of the commit message checker which
contains [this new feature][1]

Closes #15

[1]: mristin/opinionated-commit-message#59
johnboyes added a commit to SpectoLabs/hoverfly-github-action that referenced this issue Aug 15, 2020
Updated to the latest version of the commit message checker which
contains [this new feature][1]

Closes #15

[1]: mristin/opinionated-commit-message#59
johnboyes added a commit to agilepathway/label-checker that referenced this issue Aug 17, 2020
Updated to the latest version of the commit message checker which
contains [this new feature][1]

[1]: mristin/opinionated-commit-message#59
johnboyes added a commit to agilepathway/label-checker that referenced this issue Aug 17, 2020
Updated to the latest version of the commit message checker which
contains [this new feature][1]

[1]: mristin/opinionated-commit-message#59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants