finish migrating to tagspec v0.6.0 #355
Conversation
WalkthroughThis pull request refactors the public TagArg API by renaming enum variants (Var → Variable, Expr → Any) and introducing two new public enums (TokenCount and LiteralKind) to enhance metadata around template tag arguments. Updates propagate across completions, snippets, and built-in tag specifications. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant IDE as IDE Module<br/>(completions/snippets)
participant Args as Semantic Args<br/>(validation)
participant Specs as TagArg Specs
participant BuiltIns as Built-in Tags
User->>IDE: Request completion/snippet
IDE->>Args: Query argument patterns
Args->>Specs: Match TagArg::Variable/Any
Specs->>BuiltIns: Resolve tag specification
BuiltIns->>Specs: Return TagArg with<br/>TokenCount & LiteralKind
Specs->>Args: Validate with count metadata
Args->>IDE: Return processed args
IDE->>User: Display completion/snippet
Note over Specs,BuiltIns: New: count & kind metadata<br/>enriches argument semantics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/djls-semantic/src/semantic/args.rs (4)
134-142: Include the newkindfield when matching literals.
TagArg::Literalnow carries akind, so this pattern no longer compiles. Please ignore that field explicitly.- if let TagArg::Literal { lit, required } = arg { + if let TagArg::Literal { lit, required, .. } = arg {
208-238: Update matches toVariable/Anyand respectTokenCount.
TagArg::VarandTagArg::Exprwere renamed, so this match arm no longer exists and the file fails to build. Additionally, we need to branch on the newTokenCountso multi-token specs validate correctly.- TagArg::Var { .. } | TagArg::String { .. } => { - // Consume exactly 1 token - bit_index += 1; - } - TagArg::Expr { required, .. } => { + TagArg::Variable { count, .. } => { + match count { + crate::templatetags::TokenCount::Exact(n) => { + bit_index = bit_index.saturating_add(n).min(bits.len()); + } + crate::templatetags::TokenCount::Greedy => { + bit_index += 1; + } + } + } + TagArg::String { .. } => { + bit_index += 1; + } + TagArg::Any { required, .. } => {Please extend the
Anybranch to honorTokenCount::Exactin addition to the existing greedy path so the new spec metadata is enforced.
312-316: Rename the enum variants inargument_name.
TagArg::VarandTagArg::Exprno longer exist, so this match arm fails to compile. Switch toVariable/Any.- | TagArg::Var { name, .. } + | TagArg::Variable { name, .. } | TagArg::String { name, .. } - | TagArg::Expr { name, .. } + | TagArg::Any { name, .. } | TagArg::Assignment { name, .. }
612-620: Update the test to use the new constructors.This block still instantiates the removed
TagArg::Exprvariant and omits the requiredkindfield onTagArg::Literal, so the test no longer compiles. Switch to the helper constructors introduced with the v0.6.0 API.- TagArg::Expr { - name: "optional".into(), - required: true, - }, - TagArg::Literal { - lit: "reversed".into(), - required: true, - }, + TagArg::expr("optional", true), + TagArg::syntax("reversed", true),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/djls-ide/src/completions.rs(1 hunks)crates/djls-ide/src/snippets.rs(5 hunks)crates/djls-semantic/src/lib.rs(1 hunks)crates/djls-semantic/src/semantic/args.rs(8 hunks)crates/djls-semantic/src/templatetags.rs(1 hunks)crates/djls-semantic/src/templatetags/builtins.rs(18 hunks)crates/djls-semantic/src/templatetags/specs.rs(9 hunks)noxfile.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/djls-ide/src/snippets.rs (1)
crates/djls-semantic/src/templatetags/specs.rs (4)
var(378-384)syntax(353-359)modifier(362-368)expr(335-341)
crates/djls-semantic/src/semantic/args.rs (1)
crates/djls-semantic/src/templatetags/specs.rs (6)
expr(335-341)var(378-384)syntax(353-359)modifier(362-368)assignment(403-409)varargs(395-400)
🪛 GitHub Actions: bench
crates/djls-semantic/src/semantic/args.rs
[error] 783-783: cargo codspeed build failed: this file contains an unclosed delimiter (crates/djls-semantic/src/semantic/args.rs:783)
🪛 GitHub Actions: lint
crates/djls-ide/src/snippets.rs
[warning] 183-183: Diff in snippet formatting detected by cargo fmt check. Potential formatting change needed: adjust Vec initialization with .into() placement in end_tag.args and intermediate_tags.
crates/djls-semantic/src/semantic/args.rs
[error] 783-783: Unclosed delimiter reported during formatting: this file contains an unclosed delimiter at line 783. Fix syntax to proceed with fmt check.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Python 3.14, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.14, Django 6.0a1 (ubuntu-latest)
- GitHub Check: Python 3.14, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.14, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.13, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.13, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.11, Django 4.2 (ubuntu-latest)
| intermediate_tags: Cow::Borrowed(&[]), | ||
| args: vec![TagArg::Var { | ||
| name: "name".into(), | ||
| required: true, | ||
| }] | ||
| args: vec![TagArg::var("name", true)] | ||
| .into(), | ||
| }; | ||
|
|
||
| let snippet = generate_snippet_for_tag_with_end("block", &spec); | ||
| assert_eq!(snippet, "block ${1:name} %}\n$0\n{% endblock ${1} %}"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_snippet_with_end_tag() { | ||
| use std::borrow::Cow; | ||
|
|
||
| let spec = TagSpec { | ||
| module: "django.template.defaulttags".into(), | ||
| end_tag: Some(EndTag { | ||
| name: "endautoescape".into(), | ||
| required: true, | ||
| args: Cow::Borrowed(&[]), | ||
| }), | ||
| intermediate_tags: Cow::Borrowed(&[]), | ||
| args: vec![TagArg::Choice { | ||
| name: "mode".into(), | ||
| required: true, | ||
| choices: vec!["on".into(), "off".into()].into(), | ||
| }] | ||
| .into(), | ||
| }; | ||
|
|
||
| let snippet = generate_snippet_for_tag_with_end("autoescape", &spec); | ||
| assert_eq!( | ||
| snippet, | ||
| "autoescape ${1|on,off|} %}\n$0\n{% endautoescape %}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_snippet_for_url_tag() { | ||
| let args = vec![ | ||
| TagArg::String { | ||
| name: "view_name".into(), | ||
| required: true, | ||
| }, | ||
| TagArg::VarArgs { | ||
| name: "args".into(), |
There was a problem hiding this comment.
Fix the cargo fmt regression.
CI’s cargo fmt check is failing here because the newline before .into() doesn’t match rustfmt output. Please run cargo fmt (or collapse .into() onto the same line) so the lint job passes.
- args: vec![TagArg::var("name", false)]
- .into(),
+ args: vec![TagArg::var("name", false)].into(),
}),
intermediate_tags: Cow::Borrowed(&[]),
- args: vec![TagArg::var("name", true)]
- .into(),
+ args: vec![TagArg::var("name", true)].into(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| args: vec![TagArg::var("name", false)] | |
| .into(), | |
| }), | |
| intermediate_tags: Cow::Borrowed(&[]), | |
| args: vec![TagArg::Var { | |
| name: "name".into(), | |
| required: true, | |
| }] | |
| args: vec![TagArg::var("name", true)] | |
| .into(), | |
| }; | |
| args: vec![TagArg::var("name", false)].into(), | |
| }), | |
| intermediate_tags: Cow::Borrowed(&[]), | |
| args: vec![TagArg::var("name", true)].into(), | |
| }; |
🤖 Prompt for AI Agents
In crates/djls-ide/src/snippets.rs around lines 186 to 192, the formatting
mismatch is causing the CI cargo fmt failure: the newline before `.into()`
doesn't match rustfmt output; fix by running `cargo fmt` or collapsing the
`.into()` call onto the previous line so the code conforms to rustfmt (e.g.,
place `.into()` immediately after the vec construction or reformat the block)
and commit the formatted changes.
| if token.contains('=') { | ||
| break; | ||
| } | ||
| crate::templatetags::TokenCount::Greedy => { | ||
| // Assignment arguments can appear as: | ||
| // 1. Single token: var=value | ||
| // 2. Multi-token: expr as varname | ||
| // Consume until we find = or "as", or hit next literal | ||
|
|
There was a problem hiding this comment.
Remove the stray TokenCount::Greedy match fragment.
The inserted crate::templatetags::TokenCount::Greedy => { ... } is hanging inside the loop, which leaves an unclosed match arm and breaks compilation (matching the formatter error). Drop it or move the intended logic into a proper match count above.
- if token.contains('=') {
- break;
- }
- crate::templatetags::TokenCount::Greedy => {
- // Assignment arguments can appear as:
- // 1. Single token: var=value
- // 2. Multi-token: expr as varname
- // Consume until we find = or "as", or hit next literal
-
- // If we hit "as", consume one more token (the variable name)
+ if token.contains('=') {
+ break;
+ }
+
+ // If we hit "as", consume one more token (the variable name)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if token.contains('=') { | |
| break; | |
| } | |
| crate::templatetags::TokenCount::Greedy => { | |
| // Assignment arguments can appear as: | |
| // 1. Single token: var=value | |
| // 2. Multi-token: expr as varname | |
| // Consume until we find = or "as", or hit next literal | |
| if token.contains('=') { | |
| break; | |
| } | |
| // If we hit "as", consume one more token (the variable name) |
🤖 Prompt for AI Agents
In crates/djls-semantic/src/semantic/args.rs around lines 258 to 266, there is a
stray match arm fragment `crate::templatetags::TokenCount::Greedy => { ... }`
left inside a loop which leaves an unclosed match and breaks compilation; remove
this dangling fragment or relocate its logic into the existing `match count {
... }` block above so all `TokenCount` arms are handled in one place, ensuring
the match is properly closed and the loop only contains valid control flow.
| pub use specs::TagArg; | ||
| pub use specs::TagSpec; | ||
| pub use specs::TagSpecs; | ||
| pub use specs::TokenCount; |
There was a problem hiding this comment.
Expose LiteralKind to fix compile error.
lib.rs now does pub use templatetags::LiteralKind;, but this module never re-exports that symbol, so the crate won’t compile. Please export LiteralKind here alongside TokenCount.
pub use specs::TagArg;
pub use specs::TagSpec;
pub use specs::TagSpecs;
+pub use specs::LiteralKind;
pub use specs::TokenCount;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/djls-semantic/src/templatetags.rs around line 10, the module currently
only re-exports TokenCount but lib.rs expects LiteralKind to be re-exported as
well; add a pub use for LiteralKind from the appropriate source (the specs crate
or the module where LiteralKind is defined) alongside TokenCount so that lib.rs
can publicly expose templatetags::LiteralKind and the crate will compile.
Summary by CodeRabbit
Release Notes
Refactor
New Features
Chores