-
Notifications
You must be signed in to change notification settings - Fork 13
feat: add /format skill for pre-commit formatting checks #275
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
Changes from all commits
6b6bb2e
080bae1
842c523
3d2c7be
62ed301
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| --- | ||
| name: format | ||
| description: Run all formatting, linting and cleaning checks before committing code | ||
| --- | ||
|
|
||
| Run all formatting, linting, and cleaning tasks that should be done before committing code. Fix any issues found automatically where possible. | ||
|
|
||
| ## Steps | ||
|
|
||
| 1. **Rust formatting** (requires nightly): | ||
| ```bash | ||
| cargo +nightly fmt --all | ||
| ``` | ||
|
|
||
| 2. **TOML formatting**: | ||
| ```bash | ||
| taplo format --config .config/taplo.toml | ||
| ``` | ||
|
|
||
| 3. **Zepter checks** (feature propagation): | ||
| ```bash | ||
| zepter run --config .config/zepter.yaml | ||
| ``` | ||
|
|
||
| 4. **Clippy linting**: | ||
| ```bash | ||
| cargo clippy --all-targets --all-features --workspace -- -D warnings | ||
| ``` | ||
|
|
||
| ## Notes | ||
|
|
||
| - Run formatting commands (steps 1-3) first as they may auto-fix issues | ||
| - Clippy warnings should be treated as errors (`-D warnings`) | ||
| - If `taplo` or `zepter` are not installed, inform the user how to install them: | ||
| - `cargo install taplo-cli` | ||
| - `cargo install zepter` | ||
| - If nightly fmt is not installed help user install with `rustup component add --toolchain nightly-x86_64-unknown-linux-gnu rustfmt` | ||
| - Report all errors found and fix them where possible | ||
|
mudigal marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,11 @@ | |
| - NEVER add Co-Authored-By lines to commits | ||
| - NEVER use git push --force or git push -f | ||
|
|
||
| **Automatic formatting:** | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be moved to a hook
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see the value in using a git hook for automatic enforcement. However, one consideration is that adding formatting to a pre-push hook would add time to every push — the hook would run formatting on all files before allowing the push to complete. For our codebases, this could add noticeable delay (30+ seconds) to each push. IMO, the Skills.md approach provides more flexibility. |
||
| - ALWAYS run `/format` after generating or modifying Rust code | ||
| - ALWAYS run `/format` before creating any git commit | ||
| - This ensures all code follows project formatting standards (Rust, TOML, feature propagation) and passes clippy | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't these be enough without the SKILL.md ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anthropic merged commands and skills. So by having this skill you can tell claude to
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, but I didn't mean to leave
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all these commands we also have mentioned inside
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and what about bloating the context window with unnecessary stuff ? that's my biggest concern with all those skills that are introducing knowledge which is already generally available
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No it wont. If we just put it in this file and not as a skill, we are then forcing people to use claude for commit, pull, push everything. We should provide an option to the user to use claude for what they want to. With skills we will give an option for people to use /format in claude to format everything and then if they can use git commands on their own. Adding in this file (Claude.md) for those who want to use claude even for git commands.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry @mudigal but I dont understand. Could rephrase maybe ? Nothing in CLAUDE.md can force me to use Claude instead of git command if I wish to. Maybe I explained it poorly but I'm arguing that having exactly the same commands in both files Claude.md and Skill.md - doesn't bring any new value to Claude. Claude by being aware of Claude.md already knows how to format and lint and all - bringing another file to Claude looks like bloating the context for no gain
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The key distinction here is how different configuration files behave: Claude.md — Instructions here only apply when you explicitly run Claude (e.g., "commit and push"). If we document "check for formatting before committing" in Claude.md, it will only trigger when you use Claude for the commit. This is automatic behavior that Claude follows whenever it's invoked. Skills.md — This defines explicit skills that users can invoke on demand, like /format. When a user runs /format through Claude, it performs only that specific task (formatting) and nothing else. This approach gives users flexibility:
The user stays in control of what they want Claude to do. Having this choice is valuable for different workflows and preferences.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Let's be honest, the context won't blow if you add a few extra lines to it. Claude is smart enough to strip or compact the context whenever needed. If it comes to skills, only their short description is loaded into Claude's context. The full description is lazily uploaded into the context memory whenever needed, so I would not worry about skills overflowing the context window either, especially with modern context windows of 1M+ tokens |
||
|
|
||
| ## Project Overview | ||
|
|
||
| Polkadot Bulletin Chain is a specialized blockchain providing distributed data storage and retrieval infrastructure for the Polkadot ecosystem. It serves as a storage solution primarily for the People/Proof-of-Personhood chain, functioning as a bridge-connected parachain with integrated IPFS support. | ||
|
|
||
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.
I'm a bit scepitcal when it comes to any skills
Isn't this general rust knowledge of Claude to figure out these command?
Aren't commands already existing in CLAUDE.md sufficient?
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.
I can understand your scepticism but this way we're explicitly stating what we expect from Claude.
Things like linting and formatting rarely change so it won't be a maintenance burden
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.
I think CLAUDE.md is enough, I just tell format and it does all "rustfmt, taplo, zepter"
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.
Specifying explicitly wherever possible, always helps. Cant bank on Claude to assume, so explicit is better here.