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

Feature: lint PR titles #11

Open
mtojek opened this issue Nov 8, 2024 · 4 comments
Open

Feature: lint PR titles #11

mtojek opened this issue Nov 8, 2024 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@mtojek
Copy link
Member

mtojek commented Nov 8, 2024

Problem Statement

Sometimes community users forget about adjusting the PR title (and merge commit) to the style guide of the Git repository. aicommit already uses the GPT model to adjust wording of particular commits, but not the PR title.

Proposal

Let's introduce aicommit lint command which can validate the proposed PR title. The command can be executed as Github Action and fail CI job if the PR does not meet style guide requirements.

Proposed Workflow

Command: aicommit lint --pr-title "fix: set dogfood go version to match go.mod"

  1. Load the style guide rules.
  2. Prepare the prompt: Ask to lint the PR title according to the style guide. Expected result: OK or suggestion.
  3. Exit status 0 or output the suggestion.
@mtojek mtojek self-assigned this Nov 8, 2024
@coder-labeler coder-labeler bot added the enhancement New feature or request label Nov 8, 2024
@mafredri
Copy link
Member

mafredri commented Nov 8, 2024

I think aicommit is intended more like a wrapper around git commit, so I feel adding --lint capabilities breaks that expectation.

Since aicommit is its own command under cmd/, would it make sense to introduce a new cmd like aititle, ailint or aicommitlint instead?

PS. I think could be 3 states. 1) ok, no suggestion, 2) ok, improvement suggestion and 3) fail, suggestion. Not sure how we should handle this either via additional flags or output/exit codes.

@ammario
Copy link
Member

ammario commented Nov 8, 2024

The tool is really for commit messages and not PR titles, but in the case of Coder development where they're one and the same due to squash merges the difference doesn't matter. For the simplicity of this tool's scope I believe the command line should indicate it's checking / linting a commit message and not a PR title. I was also trying to avoid subcommands, so I think the best approach would be something like:

$ aicommit --lint "some commit message here"

with the exit status determining success. Potential success outputs could be:

✅ begins with (fix|perf|feat)
✅ subject less than 80 character
✅ is in imperative mood

and, failure:

❌ begins with unknown prefix "dog"
✅ subject less than 80 character
✅ is in imperative mood
suggestion: fix(perf): increase read buffer size

The checkboxes could be sent via stderr with only the raw suggestion on stdout for composability.


Using this tool to do both generation and linting means we can re-use the COMMITS.md style guide, which is nice.

@mtojek
Copy link
Member Author

mtojek commented Nov 12, 2024

I was also trying to avoid subcommands, so I think the best approach would be something like:

The reason I suggested lint subcommand is because there is a conflict with another switch, --amend. I guess it won't hurt us if we add extra condition to verify conflicting options, but it might be confusing in the future when we add more operations like linting that requires isolated switches.

The checkboxes could be sent via stderr with only the raw suggestion on stdout for composability.

In the slack thread @mafredri suggested a configurable output format to support JSON, but I guess it depends on the CI integration.

Using this tool to do both generation and linting means we can re-use the COMMITS.md style guide, which is nice.

Yup, that's the plan 👍 .

@ammario
Copy link
Member

ammario commented Nov 12, 2024

The reason I suggested lint subcommand is because there is a conflict with another switch, --amend. I guess it won't hurt us if we add extra condition to verify conflicting options, but it might be confusing in the future when we add more operations like linting that requires isolated switches.

Right now the CLI has leaned into flags all the way. If there's a point where subcommands make more sense I would rather do a major overhaul / breaking change to the CLI and convert many flags at once to commands.

In the slack thread @mafredri suggested a configurable output format to support JSON, but I guess it depends on the CI integration.

I'm game with that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants