Improvements for consideration

[derberg]

Hey, I was wondering if you consider the following improvements to this action:

• have explicit option to check if the subject starts with upper case or lower case, something like subjectUppercase. Regex is fine but not readable out of the box for everybody. It would be great to throw clear info to the user that the subject should start with an upper case or lower case. What do you think?
• turn action into a more conversational action. So whenever linting is done, publish a successful message or an error as a comment to the PR, so PR's submitter doesn't have to go to check logs.

I'm wondering what do you think about it

[amannn]

Hey @derberg, thank you for your feedback!

Regex is fine but not readable out of the box for everybody.

That's a valid point, I was thinking the same thing when I implemented the feature recently. What do you think about an additional option subjectPatternError where you can configure a custom error message that is shown if subjectPattern doesn't match?

Essentially it would be preferred to the default error here:

action-semantic-pull-request/src/validatePrTitle.js

Lines 69 to 73 in c8c1d3e

	if (!match) {
	throw new Error(
	`The subject "${result.subject}" found in pull request title "${prTitle}" doesn't match the configured pattern "${subjectPattern}".`
	);
	}

Would you be interested in contributing this feature?

turn action into a more conversational action

I'm a bit hesitant to add this to be honest. Typically you'll have multiple checks (lint, test, etc.) and these are reported as regular checks in Github. I think if each of these would comment on your PR it would likely become too much. I guess for something as important as a deployment, the comment might be viable (e.g. Vercel does this). But I think this action is likely not the most important part of your workflow, therefore I think it should keep noise to a minimum.

What do you think?

[derberg]

What do you think about an additional option subjectPatternError where you can configure a custom error message that is shown if subjectPattern doesn't match?

This would solve my use case as I need it exactly only for upper/lower case check. I'm just afraid that someone can have a use case that they want to have several patterns, with different error for each. Maybe this should be done as an array then? of course provided by user as a string and then serialized to array. So it could be subjectPattern: 'pattern' but one could do subjectPattern: 'pattern1,pattern2. And same with pattern error, and of course order matters. What do you think?

And yes, I'm super interested in contributing it as this is the only reason why I cannot use it and consider fork -> but let's face the fact, it doesn't make sense to fork if you are open for PRs

I'm a bit hesitant to add this to be honest. Typically you'll have multiple checks (lint, test, etc.) and these are reported as regular checks in Github. I think if each of these would comment on your PR it would likely become too much. I guess for something as important as a deployment, the comment might be viable (e.g. Vercel does this). But I think this action is likely not the most important part of your workflow, therefore I think it should keep noise to a minimum.

I think it is nice to put relevant info for users in the comment, and not require them to check logs. This is not a blocker for me though. In the end, it could be optional, I mean you could enable PR comments with a parameter that by default would be set to false

[amannn]

I'm just afraid that someone can have a use case that they want to have several patterns, with different error for each.

I understand. However I'm not sure how many things you really want to validate for in the title? Even if, I guess something like this would still be decent DX:

subjectPatternError: |
  The subject found in the pull request title didn't match the configured pattern.

  Please ensure that:
  1. The subject doesn't start with an uppercase character.
  2. A JIRA ticket is referenced.

  Example: "feat: add button (JIRA-1234)"

What do you think?

And yes, I'm super interested in contributing

That's great! The only thing a bit tricky with contributing to GH actions is that they can't be tested in the original repository if you create a pull request from a fork (due to the pull_request / pull_request_target event topic).

So what you could do is:

1. Fork the repo
2. Create a PR in your own repo
3. The "Lint PR title preview (current branch)" workflow will run the new version and will help you validate the change
4. Create a PR to this repo. In this case the preview workflow will fail, but we'll know that it works since you tested it in the fork.

At least that's the simplest solution I can think of 🙂 .

Let me know if I can help with something!

I think it is nice to put relevant info for users in the comment, and not require them to check logs. This is not a blocker for me though. In the end, it could be optional, I mean you could enable PR comments with a parameter that by default would be set to false

I understand. At the moment I don't see this as necessary, but if you feel strongly about this, please open a separate issue and we can see if other users are also interested and gather some feedback to possibly plan the feature.

[amannn]

Btw. I've added a PR with a small contributors guide. Contributing to GitHub actions is a bit complicated and this topic has come up a few times now. If the process above works for you or if you have any feedback for the contributors guide from your perspective, please let me know!

Thank you very much for contributing! 🙂

[derberg]

What do you think?

yeah, better not assume a complex scenario if nobody yet asked about it 😄
and as you wrote, error can be super descriptive

Let me know if I can help with something!

no worries man, I wrote few actions already and dozen of workflows and feel the pain of testing. I have already a test repo where I test all my actions and workflows so now how to simulate a real environment. When I'll create a PR I make sure to reference the manual test I used for testing.

[amannn]

Sounds great, thanks!

and feel the pain of testing

Oh man, good to hear I'm not missing something obvious 😅 .

[derberg]

@amannn ready for review. Let me know if something is missing or not as you'd like to have it.

[amannn]

Out in v3.2.0 – thank you!