Skip to content

Conversation

@nyurik
Copy link
Member

@nyurik nyurik commented Nov 1, 2025

No description provided.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements support for relational attributes in DBC file parsing, which previously resulted in NotImplemented errors. The changes enable parsing of BA_DEF_REL_, BA_DEF_DEF_REL_, and BA_REL_ constructs that define and store attributes for relationships between nodes, messages, and signals.

  • Added three new AST types to represent relational attributes and their definitions
  • Integrated relational attribute parsing into the main DBC parser
  • Updated snapshot tests to reflect successful parsing instead of errors
  • Refactored shared parsing logic in AttributeDefinition into a reusable function

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/ast/mod.rs Exports new relational attribute modules
src/ast/dbc.rs Adds fields for relational attributes and integrates parsing logic
src/ast/attribute_definition_for_relation.rs New module defining AttributeDefinitionForRelation struct
src/ast/attribute_value_for_relation.rs New module defining AttributeValueForRelation struct
src/ast/attribute_value_for_relation_type.rs New module defining AttributeValueForRelationType enum
src/ast/attribute_definition.rs Refactors parsing logic into reusable parse_obj_type_vals function
src/ast/attribute_default.rs Updates to handle both regular and relational attribute defaults
tests/snapshots/dbc-cantools/*.snap Updated snapshots showing successful parsing of relational attributes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@tegimeki tegimeki left a comment

Choose a reason for hiding this comment

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

Looking good so far, it works on some of the trickier private DBCs which use BA_DEF_DEF_REL and other less-common ones (which never worked w/the original crate)!

I'm going to test and read through some more and let others do the same, then am inclined to approve.

Is the codecov percentage for the new changes a concern?

Copy link
Contributor

@DanielT DanielT left a comment

Choose a reason for hiding this comment

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

Looks good to me.
You'll want to make the fields of the new structs pub though.

@nyurik nyurik merged commit 570d451 into oxibus:main Nov 2, 2025
7 checks passed
@nyurik nyurik deleted the rel4 branch November 2, 2025 17:39
@nyurik nyurik mentioned this pull request Nov 1, 2025
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.

3 participants