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

feat!: show input: from a new line #4102

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

abitrolly
Copy link
Contributor

@abitrolly abitrolly commented Jul 20, 2024

This will be less confusing to read, and also easier to test and copy/paste (for example for #4101).

This error log is confusing for people not familiar with commitlint.

⧗   input: fix: ingest - do not double strip /doc/ prefix
It is already stripped here
https://gitlab.com/gitlab-community/modelops/applied-ml/code-suggestions/ai-assist/-/blob/22bb9cf2b996dc2ec347bad761abb12fd11bcf75/scripts/ingest/gitlab-docs/steps/parse.rb#L16
✖   body's lines must not be longer than 100 characters [body-max-line-length]
✖   found 1 warnings

This one should be more clear.

⧗   --- input ---
fix: ingest - do not double strip /doc/ prefix

It is already stripped here
https://gitlab.com/gitlab-community/modelops/applied-ml/code-suggestions/ai-assist/-/blob/22bb9cf2b996dc2ec347bad761abb12fd11bcf75/scripts/ingest/gitlab-docs/steps/parse.rb#L16
✖   body's lines must not be longer than 100 characters [body-max-line-length]
✖   found 1 warnings

Or maybe even this one.

⧗   commit message:
fix: ingest - do not double strip /doc/ prefix

It is already stripped here
https://gitlab.com/gitlab-community/modelops/applied-ml/code-suggestions/ai-assist/-/blob/22bb9cf2b996dc2ec347bad761abb12fd11bcf75/scripts/ingest/gitlab-docs/steps/parse.rb#L16
✖   body's lines must not be longer than 100 characters [body-max-line-length]
✖   found 1 warnings

Motivation and Context

Error output should not mess up original commit message, because it is hard to understand what is wrong.

Usage examples

// commitlint.config.js
module.exports = {};
echo "your commit message here" | commitlint # fails/passes

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

This will be less confusing to read, and also easier to test
and copy/paste..
Copy link

codesandbox-ci bot commented Jul 20, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@escapedcat
Copy link
Member

Thanks! I understand that this might be a bit confusing for first time users but this is not a common issue we get. So I would rather close this. It doesn't justify a breaking change.

@abitrolly
Copy link
Contributor Author

@escapedcat this was confusing for 16k users who starred this and will be confusing for a 100k+ more who just don't know what GitHub is. If the confusion if just 5 minutes, it is still worth the output breaking change for me.

For integration with editors and stuff, I guess commitlint provides a proper machine readable output instead of this.

@escapedcat
Copy link
Member

Thanks for your feedback. Might be good to open up an issue for this to discuss this further and get some feedback from others. Try searching if we maybe have similiar issues already. Thanks!

@abitrolly
Copy link
Contributor Author

@escapedcat why this issue is not suitable to discuss it?

@escapedcat
Copy link
Member

@escapedcat why this issue is not suitable to discuss it?

Hm, no, it is, but I would prefer you open up an official issue for this to get some feedback/input from other users.

@abitrolly
Copy link
Contributor Author

@escapedcat created #4112

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants