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: Add more thread context preserving combinators #230

Conversation

OlaoluwaM
Copy link
Contributor

No description provided.

@OlaoluwaM OlaoluwaM requested a review from pbrisbin February 13, 2025 21:28
@OlaoluwaM OlaoluwaM self-assigned this Feb 13, 2025
@OlaoluwaM OlaoluwaM requested a review from a team as a code owner February 13, 2025 21:28
pbrisbin
pbrisbin previously approved these changes Feb 13, 2025
Copy link
Member

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but this is an addition, which would be feat:, not refactor:.

(We're also not using semantic-release in this project yet, FYI)

@OlaoluwaM OlaoluwaM changed the title refactor: Add more thread context preserving combinators feat: Add more thread context preserving combinators Feb 13, 2025
@OlaoluwaM OlaoluwaM force-pushed the ola/add-more-async-combinators-that-maintain-parent-thread-context branch from f7d1d2e to 058c146 Compare February 13, 2025 21:47
@OlaoluwaM OlaoluwaM enabled auto-merge (squash) February 13, 2025 21:50
@OlaoluwaM OlaoluwaM disabled auto-merge February 13, 2025 21:51
@OlaoluwaM OlaoluwaM requested a review from pbrisbin February 14, 2025 17:54
Copy link
Member

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a further note about conventional-commits, since I've been reading up on it and we may start using it more...

You can't just make up any prefix like refactor: or ci:. The team would have to agree on a convention (one we make up or one we adopt). By far the most common is Angular, which only has chore:, fix:, or feat:. I see no real reason to deviate.

You do, however, have (scope) available. So you'd probably want to do chore(ci): instead of ci:.

I think that even if we adopt a loose "use conventional commits if you want", those commits that are doing so should probably do so correctly.

@pbrisbin
Copy link
Member

That said, just because you were making that commit in response to a CI failure, that doesn't mean the change itself has anything to do with CI, such as working on the workflow itself (which is what I'd take chore(ci): to mean). Those commits might be best prefixed by chore(http):, but I usually just do chore: anyway for anything that doesn't change library API 🤷

This stuff is tricky!

@OlaoluwaM
Copy link
Contributor Author

You can't just make up any prefix like refactor: or ci:. The team would have to agree on a convention (one we make up or one we adopt). By far the most common is Angular, which only has chore:, fix:, or feat:. I see no real reason to deviate.

My bad, I've always used these prefixes since they seem to be recommended in the conventional commits spec. It's something I've always done. I have a git commit message template with all these things outlined so I've made a habit of just picking based on context/occasion, but I guess now that we're trying to adopt this style commit messages at an org level, I ought to have asked/looked into the particular variant we're looking to follow.

@OlaoluwaM
Copy link
Contributor Author

That said, just because you were making that commit in response to a CI failure, that doesn't mean the change itself has anything to do with CI, such as working on the workflow itself (which is what I'd take chore(ci): to mean). Those commits might be best prefixed by chore(http):, but I usually just do chore: anyway for anything that doesn't change library API 🤷

Yeah, the spec is pretty open-ended about the meaning of prefixes beyond "fix", "feat", and "BREAKING CHANGE", so I can easy how it might be easy to have conflicting definitions

@pbrisbin
Copy link
Member

used these prefixes since they seem to be recommended in the conventional commits spec

Aha, see this is why I make these comments. I never saw this,

types other than fix: and feat: are allowed, for example @commitlint/config-conventional (based on the Angular convention) recommends build:, chore:, ci:, docs:, style:, refactor:, perf:, test:, and others.

Nevermind then, TIL!

@OlaoluwaM OlaoluwaM merged commit 1f0ef82 into main Feb 14, 2025
8 checks passed
@OlaoluwaM OlaoluwaM deleted the ola/add-more-async-combinators-that-maintain-parent-thread-context branch February 14, 2025 20:09
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