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

chore: setup config semantical commits #169

Closed
wants to merge 26 commits into from
Closed

chore: setup config semantical commits #169

wants to merge 26 commits into from

Conversation

MarlonPassos-git
Copy link
Contributor

Tip

The owner of this PR can publish a preview release by commenting /publish in this PR. Afterwards, anyone can try it out by running pnpm add radashi@pr<PR_NUMBER>.

Summary

After the comment on the issue:

"Conventional commits: I've added a commit message linter to ensure that all commits follow the conventional commits specification."

I noticed the interest in standardizing commit messages. I set up a basic configuration using Husky and commitlint to validate commit messages, both in the pre-commit stage and in CI.

We can refine and add extra rules in the future, but this PR serves as a starting point for that standardization.

Related issue, if any:

For any code change,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed
  • Related benchmarks have been added or updated, if needed

Does this PR introduce a breaking change?

No (maybe for contributors 🤣).

MarlonPassos-git and others added 26 commits July 16, 2024 23:02
- Keep the tests in one file. Use `vi.resetModules` to check for correct behavior when globalThis.AggregateError is undefined and when it‘s not.

- Inline the polyfill so it‘s not instantiated unless necessary.

- Rename the local const to `AggregateErrorOrPolyfill` then use `export { … as … }` to export it as `AggregateError`. This prevents ESBuild from renaming the polyfill to `AggregateError2`, which it does to prevent variable shadowing.
@MarlonPassos-git
Copy link
Contributor Author

Now that I've seen that some commits from the past were in an unacceptable standard. (I fell into my own trap 😄)

In this case, I don't know if it would be valid to rewrite old commits from this PR to fit the standard or maybe just skip it this time.

The funny thing is that even though I've merged with the main repository, several old commits appear in my PRs

@aleclarson
Copy link
Member

Thanks for the PR, Marlon! I'll have to decline this suggestion, and here's why: I have a policy to always squash-merge1 pull requests. This gives contributors the freedom to write whatever suits them when committing to their PR, without a pre-commit/pre-push hook “scolding” them. When squash-merging a PR, GitHub uses the PR title as the commit message. I've already set up an automated check that will prevent merging if a PR title does not conform to the Conventional Commits specification. Hopefully that all made sense! 😄

By the way, I want to make sure you know that I really do appreciate the effort you've been putting in. It's really my fault for not better documenting the project setup, including things I've already said "no" to. Keep up the great work!

Footnotes

  1. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-commits

@aleclarson aleclarson closed this Aug 15, 2024
@aleclarson
Copy link
Member

aleclarson commented Aug 15, 2024

P.S. It appears your fork's main branch is out-of-sync. I would recommend running this to get that fixed:

git remote add radashi-org https://github.com/radashi-org/radashi
git rebase radashi-org main -X theirs
git push --force

If that fails, you may want to hard reset to radashi-org/main. If you have commits of your own on the main branch, you'll probably want to consider keeping a separate branch for them in case this happens again. 😄

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 this pull request may close these issues.

2 participants