-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add Conventional Commit linter to CI #108
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #108 +/- ##
=======================================
Coverage 75.60% 75.60%
=======================================
Files 30 30
Lines 1496 1496
Branches 64 64
=======================================
Hits 1131 1131
Misses 301 301
Partials 64 64 ☔ View full report in Codecov by Sentry. |
cf7804d
to
e3c3178
Compare
@ships Can you check my GH Action, and let me know if I got the nix setup part right? I'm unclear on when I can use I got a lot of 429 Too Many Requests errors until the cache filled up. Is there a way around that? Finally, I ran @zane I set the rules to match your existing style. Generally speaking, do you want this GH Action to warn people, or actually block merging? And are their other repos this should be applied to? |
@KingMob Thank you for this! It should block merging, and we should apply this to all the GenSQL / InferenceQL repos. Basically the ones that are already using this commit style. |
4f8471d
to
01092a3
Compare
Edit: NM, I found Ulli's complete list in Notion. |
@ships Does the nix stuff look right? Do I need the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, commented on optional verbosity flag for human readers.
@@ -54,7 +54,7 @@ | |||
in { | |||
# development shell | |||
devShells.default = pkgs.mkShell { | |||
buildInputs = [ pkgs.openjdk21 pkgs.clojure pkgs.babashka depsCache ] ++ (basicToolsFn pkgs); | |||
buildInputs = [ pkgs.openjdk21 pkgs.clojure pkgs.babashka pkgs.nodejs_20 depsCache ] ++ (basicToolsFn pkgs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notably, there are also nodePackages
available in nixpkgs that we currently don't use (preferring to make you npm install
). But in theory we could add those packages here too. This change makes sense to me as is though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I wasn't sure. I see it in nixpkgs now.
I feel like incompleteness is always such a problem. Like, if there's any package not available in nix, you have to fall back to package.json anyway, and then... what? Merges and conflicts? Human sacrifice, dogs and cats living together, mass hysteria!
@zane After merging, I'd like to give it a bit of time, make sure it fits smoothly into our workflow, and then roll out to other repos after a week or two. |
01092a3
to
7f3fee1
Compare
7f3fee1
to
751a683
Compare
No description provided.