migrate to TagSpecs v0.6.0 spec #350
Conversation
for more information, see https://pre-commit.ci
CodSpeed Performance ReportMerging #350 will not alter performanceComparing Summary
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughUpgrades TagSpecs to v0.6.0 hierarchical format with libraries, adds a deprecated v0.4.0 legacy compatibility converter and deserializer, changes Settings.tagspecs to a single TagSpecDef, expands TagSpecs types/exports, updates semantic conversions and block end-tag semantics, removes old TAGSPECS.md, and refreshes docs, changelog, and tooling config. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Config Source
participant Deser as Custom Deserializer
participant Compat as Legacy Compat
participant Settings as Settings
Config->>Deser: Load tagspecs (v0.6.0 hierarchical or v0.4.0 flat)
Deser->>Deser: Detect format
alt v0.6.0 (hierarchical)
Deser->>Settings: Parse to TagSpecDef
Note over Settings: TagSpecDef contains libraries
else v0.4.0 (legacy flat)
Deser->>Compat: convert_legacy_tagspecs(legacy)
Compat->>Compat: Group by module, transform tags/args, store choices in extra
Compat-->>Deser: Return TagSpecDef
Deser-->>Settings: Emit deprecation warning and provide TagSpecDef
end
Settings->>Settings: Apply overrides via tagspecs.libraries
sequenceDiagram
participant Semantic as Semantic Engine
participant Settings as Settings
participant Lib as Library Loop
participant Tag as Tag Loop
participant Convert as TagDef→TagSpec
Semantic->>Settings: settings.tagspecs()
Settings-->>Semantic: TagSpecDef (with libraries)
loop for each library
Semantic->>Lib: iterate library
loop for each tag in library
Lib->>Tag: pass (TagDef, module)
Tag->>Convert: From<(TagDef, module)>
Convert->>Convert: derive end_tag by TagType (synthesize when needed)
Convert->>Convert: map intermediates & args
Convert->>Convert: extract Choice from extra metadata if present
Convert-->>Tag: TagSpec
Tag->>Semantic: register/override TagSpec
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)crates/djls-conf/src/lib.rs (1)
⏰ 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). (20)
🔇 Additional comments (7)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)crates/djls-conf/Cargo.toml(1 hunks)crates/djls-conf/TAGSPECS.md(0 hunks)crates/djls-conf/src/lib.rs(9 hunks)crates/djls-conf/src/tagspecs.rs(2 hunks)crates/djls-conf/src/tagspecs/legacy.rs(1 hunks)crates/djls-semantic/Cargo.toml(1 hunks)crates/djls-semantic/src/templatetags/specs.rs(10 hunks)docs/configuration.md(1 hunks)docs/tagspecs.md(1 hunks)
💤 Files with no reviewable changes (1)
- crates/djls-conf/TAGSPECS.md
🔇 Additional comments (24)
crates/djls-conf/Cargo.toml (1)
12-12: LGTM!The addition of
serde_jsonandtracingworkspace dependencies appropriately supports the TagSpecs v0.5.0 migration, enabling JSON serialization for the new hierarchical format and structured logging for deprecation warnings.Also applies to: 15-15
docs/configuration.md (2)
164-164: LGTM!The updated default description is clearer and more user-friendly than the previous "
[]" notation.
168-169: Verify the migration guide link path.Similar to the CHANGELOG, this references
../crates/djls-conf/TAGSPECS.md#migration-from-v040. Please ensure this file exists at that location or update the link to point totagspecs.mdin the same directory if that's the intended target.The same verification script from the CHANGELOG comment applies here.
crates/djls-semantic/Cargo.toml (1)
22-22: LGTM!Adding
serde_jsonas a dev-dependency appropriately supports JSON-based test fixtures for the new TagSpecs format.docs/tagspecs.md (1)
1-339: Excellent comprehensive documentation!This is well-structured TagSpecs documentation with clear examples, migration guidance, and a deprecation timeline. The migration section (lines 172-339) provides detailed examples for common patterns.
However, note that this file is located at
docs/tagspecs.md, but other files in this PR referencecrates/djls-conf/TAGSPECS.md. Please ensure all references point to the correct location.crates/djls-semantic/src/templatetags/specs.rs (7)
2-2: LGTM!The
Borrowed as Balias simplifies the synthesized end tag creation on lines 196-197.
158-168: LGTM!The updated conversion correctly iterates through the new hierarchical structure (
tagspec_doc.libraries→library.tags) and constructs TagSpecs using the tuple-based From implementation with module information.
340-383: Verify the Choice argument extra metadata parsing.The Choice variant extraction (lines 361-380) relies on:
- The
extrafield containing aHashMap- The
"choices"key being present- The value being a JSON array of strings
Consider:
- Error handling: If the extra metadata is malformed or missing required fields, the code silently falls back to an empty choices list (
unwrap_or_default()). This could hide configuration errors.- Type safety: The parsing uses runtime JSON value inspection rather than typed deserialization.
Should malformed Choice arguments (missing or invalid
extra.choices) generate a warning or diagnostic rather than silently defaulting to empty choices?
396-396: LGTM!The required/optional inversion is correctly implemented and well-documented with an inline comment. The v0.5.0 spec uses
requiredwhile the runtime usesoptional, so!value.requiredis the correct mapping.
758-861: LGTM!The test coverage for conversion functions is comprehensive:
- Variable, Choice, Literal argument conversions
- End tag conversion with required/optional inversion
- Intermediate tag conversion
- Full TagDef → TagSpec conversion with module
The tests correctly verify the new v0.5.0 types and field mappings.
864-949: LGTM!The integration test comprehensively verifies:
- Built-in specs are preserved
- User-defined tags from
[[tagspecs.libraries]]are loaded correctly- Module, end tag, intermediates, and args are properly converted
- User specs can override built-in specs (the
iftag override test)- Required/optional inversion works correctly in the full pipeline
The TOML fixture uses the new v0.5.0 hierarchical format, confirming the entire conversion path works end-to-end.
187-227: Let me verify the actual tag configurations and Django's standard practices:Automatic end tag synthesis for Block tags is correctly implemented and follows established Django conventions.
The implementation is sound. Django's block template tags use the format
{% block name %}...{% endblock %}, following theendXnaming pattern consistently across all built-in tags.The code's synthesis logic is defensive—it only generates a default end tag when none is explicitly provided, and explicit definitions override the default. The existing codebase shows all Block tags already include explicit end definitions (e.g., "endblock", "endcustom", "endmytag"), confirming the endX convention is universal in the codebase.
No Django or third-party tags violate this naming pattern, and the implementation aligns with v0.5.0 spec expectations.
crates/djls-conf/src/tagspecs/legacy.rs (4)
19-97: LGTM! Legacy structures are well-defined.The legacy data structures accurately model the v0.4.0 flat format with appropriate field names, defaults, and serde attributes. The use of
#[serde(untagged)]forLegacyArgTypeDefcorrectly handles the Simple vs Choice variants.
99-214: Conversion logic is sound and well-structured.The conversion functions properly handle:
- Grouping tags by module into libraries
- Inverting
optionaltorequiredfor end tags- Mapping legacy argument types to new kinds (String/Variable → Variable, Expression/VarArgs → Any)
- Storing Choice values in extra metadata per v0.5.0 spec
The hardcoded defaults (version="0.5.0", engine="django", arg_type=Both) are appropriate for legacy migration.
216-420: LGTM! Comprehensive test coverage.The tests thoroughly validate:
- Tag type detection (standalone vs block)
- End tag required/optional inversion
- Argument kind mappings
- Choice metadata storage
- Module-based grouping
- Intermediate tag conversion
1-6: Inconsistency in deprecation timeline confirmed between files.The deprecation timeline inconsistency is verified.
crates/djls-conf/src/tagspecs/legacy.rs(line 1) specifies removal in v5.2.7, whilecrates/djls-conf/src/tagspecs.rs(line 5) specifies removal in v5.2.2. These conflicting versions must be aligned to avoid confusion about the actual deprecation schedule. Determine which timeline is authoritative and update both deprecation comments accordingly.crates/djls-conf/src/lib.rs (5)
36-52: LGTM! Well-implemented log directory function.The
log_dirfunction properly:
- Uses XDG cache directory when available
- Falls back to
/tmpgracefully- Creates the directory if needed
- Provides clear error context
The new public exports appropriately expand the API surface for the v0.5.0 hierarchical format.
80-110: LGTM! Robust deserializer with proper deprecation handling.The custom deserializer correctly:
- Prioritizes the new v0.5.0 format (optimal path)
- Falls back to legacy v0.4.0 with a clear deprecation warning
- Includes a migration guide link in the warning
- Returns a descriptive error if both formats fail
74-75: LGTM! Settings structure properly migrated to v0.5.0.The changes correctly:
- Use
TagSpecDefas the root type (instead ofVec<TagSpecDef>)- Wire in the custom deserializer with deprecation support
- Check
overrides.tagspecs.librariesfor non-empty (appropriate for hierarchical structure)- Update the accessor return type to
&TagSpecDefAlso applies to: 128-129, 204-206
543-847: LGTM! Thorough test coverage for v0.5.0 format.The updated tests properly validate:
- Hierarchical structure (libraries → tags → end/intermediates/args)
- All field types and nesting levels
- All
ArgKindDefvariants- Integration with other settings
The TOML syntax correctly reflects the new schema.
849-1002: LGTM! Comprehensive legacy format test coverage.The legacy tests thoroughly validate backward compatibility:
- Flat array to hierarchical conversion
- Module-based grouping
- Optional→required inversion for end tags
- Choice argument conversion with extra metadata
- Multiple tags per module handling
crates/djls-conf/src/tagspecs.rs (3)
8-68: LGTM! Well-designed hierarchical structures.The root structures (
TagSpecDef,TagLibraryDef,TagDef) properly model the v0.5.0 spec with:
- Clear hierarchical organization (root → libraries → tags)
- Extensibility via
extrafields- Proper serde defaults and field renaming
- Comprehensive field documentation
70-181: LGTM! Comprehensive and well-structured type definitions.The supporting structures and enums effectively model:
- Tag classification (
TagTypeDef: Block/Loader/Standalone)- End tag behavior with required field
- Intermediate tag constraints (min/max/position)
- Argument typing (positional/keyword/both) and semantic kinds
- Position constraints (Any/Last)
The separation of
arg_type(positional vs keyword) andkind(semantic role) provides clear, orthogonal classification.
183-201: LGTM! Default functions provide sensible values.The default functions correctly return:
- Version "0.5.0" (current spec version)
- Engine "django" (appropriate default)
- Boolean true (for required fields)
- PositionDef::Any (most permissive)
- ArgTypeDef::Both (most flexible)
These defaults align well with the spec's intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/djls-conf/src/tagspecs/legacy.rs (1)
96-98: Consider reusingdefault_truefrom parent module.This function duplicates the same helper already defined in the parent
tagspecsmodule (lines 190-192). You can import it from the parent module instead.Apply this diff to remove the duplication:
+use super::default_true; + -fn default_true() -> bool { - true -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/djls-conf/src/tagspecs/legacy.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/djls-conf/src/tagspecs/legacy.rs (1)
crates/djls-conf/src/tagspecs.rs (1)
default_true(191-193)
🪛 GitHub Actions: lint
crates/djls-conf/src/tagspecs/legacy.rs
[warning] 5-5: Diff in legacy.rs:5 indicates formatting changes suggested by rustfmt (code style issue).
[error] 5-5: cargo +nightly fmt --all -- --check failed. Rust code is not properly formatted according to rustfmt. Run 'cargo +nightly fmt' to fix formatting.
🪛 GitHub Check: rustfmt
crates/djls-conf/src/tagspecs/legacy.rs
[warning] 5-5:
Diff in /home/runner/work/django-language-server/django-language-server/crates/djls-conf/src/tagspecs/legacy.rs
⏰ 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). (20)
- GitHub Check: Python 3.13, Django main (ubuntu-latest)
- GitHub Check: Python 3.14, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.14, Django main (ubuntu-latest)
- GitHub Check: Python 3.14, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.10, 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)
- GitHub Check: Python 3.13, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.12, Django main (ubuntu-latest)
- GitHub Check: Python 3.10, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.13, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.13, Django 6.0a1 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 6.0a1 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.11, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.10, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.11, Django 5.2 (ubuntu-latest)
- GitHub Check: benchmarks
joshuadavidthomas
changed the title
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/djls-conf/src/tagspecs/legacy.rs (1)
5-5: Critical: Address rustfmt formatting issue (previously flagged).As noted in a previous review, line 5 has a formatting issue that does not conform to rustfmt standards. This will cause CI pipeline failures.
Please run the following command to fix all formatting issues:
cargo +nightly fmt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.mkdocs.yml(1 hunks)CHANGELOG.md(1 hunks)crates/djls-conf/src/lib.rs(9 hunks)crates/djls-conf/src/tagspecs.rs(2 hunks)crates/djls-conf/src/tagspecs/legacy.rs(1 hunks)crates/djls-semantic/src/templatetags/specs.rs(10 hunks)docs/configuration.md(1 hunks)docs/tagspecs.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/configuration.md
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (2)
crates/djls-conf/src/lib.rs (1)
crates/djls-conf/src/tagspecs/legacy.rs (1)
convert_legacy_tagspecs(104-131)
crates/djls-conf/src/tagspecs.rs (1)
crates/djls-conf/src/tagspecs/legacy.rs (1)
default_true(96-98)
🪛 LanguageTool
docs/tagspecs.md
[style] ~89-~89: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 4152 characters long)
Context: ...s extra.choices) ## Common Patterns !!! note Examples below use djls.toml...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
docs/tagspecs.md
95-95: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
119-119: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
139-139: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
161-161: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
215-215: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
226-226: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
249-249: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
262-262: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
289-289: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
300-300: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
⏰ 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). (4)
- GitHub Check: Python 3.14, Django main (ubuntu-latest)
- GitHub Check: Python 3.13, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 6.0a1 (ubuntu-latest)
- GitHub Check: Python 3.10, Django 4.2 (ubuntu-latest)
🔇 Additional comments (21)
.mkdocs.yml (1)
23-25: LGTM! Tabbed extension configured appropriately.The addition of the
pymdownx.tabbedextension withalternate_styleandcombine_header_slugoptions enables tabbed content in the documentation, which is used effectively indocs/tagspecs.mdto show configuration examples in bothdjls.tomlandpyproject.tomlformats.crates/djls-conf/src/lib.rs (6)
16-16: LGTM! Public API appropriately expanded.The new public exports for
ArgKindDef,PositionDef,TagDef,TagLibraryDef, andTagTypeDefappropriately expose the v0.6.0 schema types needed by downstream crates likedjls-semantic.Also applies to: 21-21, 25-25, 27-30
44-52: LGTM! Well-implemented utility function.The
log_dir()function provides a clean API for obtaining the log directory with appropriate fallback behavior and error handling.
128-130: LGTM! Override logic correctly updated.The override handling correctly checks
overrides.tagspecs.libraries.is_empty()to align with the new hierarchicalTagSpecDefstructure.
204-206: LGTM! Accessor signature correctly updated.The
tagspecs()method now returns&TagSpecDefinstead of&[TagSpecDef], correctly reflecting the change from a flat list to a hierarchical document structure.
547-608: LGTM! Tests comprehensively updated.The tests correctly validate the new hierarchical TagSpecDef structure with library grouping, and the test assertions properly access
libraries[0].tags[0]etc.
849-1002: LGTM! Excellent legacy format test coverage.The new
legacy_formattest module provides comprehensive validation of the v0.4.0 to v0.6.0 conversion, including:
- Flat format conversion to hierarchical structure
- End tag optional/required inversion
- Choice argument conversion with extra metadata
- Module-based grouping
crates/djls-semantic/src/templatetags/specs.rs (5)
153-177: LGTM! Library-based tag conversion correctly implemented.The
From<&Settings>implementation properly iterates throughtagspec_doc.librariesand converts each tag with its module, then merges user specs to override built-ins. The conversion flow is clean and correct.
187-227: LGTM! Tag type handling and end tag synthesis correct.The conversion properly handles the three tag types:
- Block: synthesizes
end{tagname}if not provided- Loader: uses provided end or None
- Standalone: enforces no end tag
The default values for synthesized end tags (
optional: false, empty args) are appropriate.
340-383: LGTM! Argument kind mappings correctly implemented.The conversion properly maps the new
ArgKindDefvariants to runtimeTagArgtypes:
- Literal/Syntax/Modifier unified as
TagArg::Literal(appropriate for token-based args)- Choice correctly extracts choices from
extra.metadata["choices"]- Graceful handling of missing metadata with
unwrap_or_default()
392-404: LGTM! Required/optional inversion correctly implemented.The conversion properly inverts the semantics:
requiredin the config spec becomesoptionalin the runtime representation. The comment clearly documents this intentional inversion.
757-949: LGTM! Comprehensive conversion test coverage.The tests thoroughly validate:
- Individual type conversions (TagArgDef, EndTagDef, IntermediateTagDef)
- Full TagDef to TagSpec conversion with module
- Settings integration with library grouping
- Override behavior (user specs override built-ins)
- Required/optional semantic inversion
crates/djls-conf/src/tagspecs/legacy.rs (5)
21-99: LGTM! Legacy structures correctly defined.The legacy data structures accurately represent the v0.4.0 flat format with appropriate serde attributes and defaults. The
default_true()helper correctly provides the legacy default for therequiredfield.
105-132: LGTM! Conversion correctly groups tags by module.The
convert_legacy_tagspecsfunction properly:
- Groups legacy tags by module using HashMap
- Converts grouped tags into
TagLibraryDefstructures- Creates a valid v0.6.0
TagSpecDefroot document
134-173: LGTM! Conversion helpers handle structure differences correctly.The helper functions properly:
- Determine tag type (Block if has end_tag, else Standalone)
- Invert end tag optional→required semantics
- Provide appropriate defaults for new v0.6.0 fields (min, max, position)
175-211: LGTM! Argument type mappings correctly handle legacy to v0.6.0 conversion.The conversion properly:
- Maps legacy types to new
ArgKindDefvariants- Unifies String/Variable → Variable (reasonable for template context)
- Unifies Expression/VarArgs → Any (appropriate for flexible arguments)
- Stores Choice values in
extrametadata as required by v0.6.0 spec
217-417: LGTM! Comprehensive test coverage for legacy conversion.The tests thoroughly validate:
- Simple and block tag conversion
- Optional→required semantic inversion
- All argument type mappings
- Choice metadata extraction and storage
- Module-based grouping
- Intermediate tag conversion
crates/djls-conf/src/tagspecs.rs (4)
32-68: LGTM! Hierarchical structure correctly defined.The
TagLibraryDefandTagDefstructures properly implement the v0.6.0 hierarchical format with library-based grouping, appropriate fields, and correct serde attributes.
70-128: LGTM! Type classification and constraint fields well-designed.The enum definitions (
TagTypeDef,PositionDef) and structure expansions (EndTagDef,IntermediateTagDef) properly model the v0.6.0 specification with appropriate constraints (min/max occurrences, positioning) and semantic fields.
130-181: LGTM! Argument modeling comprehensively handles semantic roles.The
TagArgDefstructure and associated enums (ArgTypeDef,ArgKindDef) provide clear separation between:
- Positional/keyword designation (
ArgTypeDef)- Semantic role (
ArgKindDef: Any, Assignment, Choice, Literal, Modifier, Syntax, Variable)The
extrafield allows for metadata storage (e.g., choices for Choice kind), and all fields have appropriate defaults.
183-201: LGTM! Default functions provide appropriate values.The helper functions correctly provide defaults for serde deserialization:
default_version(): "0.6.0"default_engine(): "django"default_position():PositionDef::Anydefault_arg_type():ArgTypeDef::BothThese align with the v0.6.0 specification expectations.
Summary by CodeRabbit
New Features
Deprecated
Compatibility
Behavior Changes
Documentation