Skip to content

Conversation

@JohannesHatke
Copy link

I saw that Attribute Definitions were missing so i added them.
I also added a few tests and changed the relevant ones.
The enum members now all contain a String for the name of the attribute.

jannes922 added a commit to jannes922/can-dbc that referenced this pull request May 26, 2025
@JohannesHatke JohannesHatke reopened this Aug 5, 2025
@marcelbuesing
Copy link
Collaborator

Hey Johannes, thank you for bearing with me. This looks great! Would you mind fixing those small CI issues?

@JohannesHatke
Copy link
Author

Sure, i'll get on that. Sorry for the open and close, just a misclick :D

@JohannesHatke
Copy link
Author

This is my first time working with github actions and unfortunately the rust toolchain that's used in the ci is very different from my local one (newest stable).
Thats why i deleted a lot in the deny.toml file. All of the warnings in the cargo deny actions are now depreceated.
The fmt changes made no sense to me, because without the take_till import it wont compile. Could you take a look at that?

@nyurik
Copy link
Member

nyurik commented Oct 18, 2025

I wonder if these would be better to do via the new lexer https://github.com/oxibus/can-dbc-pest - the structs might be similar, but all the lexing logic would be in the pest crate

@codecov
Copy link

codecov bot commented Nov 1, 2025

Codecov Report

❌ Patch coverage is 76.59574% with 11 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/ast/attribute_definition.rs 70.27% 2 Missing and 9 partials ⚠️

📢 Thoughts on this report? Let us know!

@nyurik
Copy link
Member

nyurik commented Nov 1, 2025

I reworked this PR, but there are now a few things we must fix before it gets merged:

  • big_numbers.dbc no longer parses because of this number that it tries to parse as integer
BA_DEF_ BU_  "TheNodeAttribute" INT -9.22337203685478E+018 1.84467440737096E+019;
BA_DEF_ SG_ "GenSigMissingSourceValue" INT 0 1e+09;

@nyurik nyurik requested review from DanielT and tegimeki November 2, 2025 04:33
@nyurik
Copy link
Member

nyurik commented Nov 2, 2025

@DanielT @tegimeki do we want to merge this before #64, as it changes how values are stored? That said, we may want to merge #65 to handle integers first - and in theory it should handle the above case?

Copy link

@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.

This looks fine, no obvious issues here.
I haven't run this, so my review is purely a code review.

@DanielT
Copy link

DanielT commented Nov 2, 2025

My preference would be to merge your change first - the BA_REL stuff prevents real files from being parsed at all.

Then we can fix up this change and mine in any order.

The attribute parsing added here is something I suspect just doesn't matter for most use-cases. Presumably everyone also only looking for attributes they already know about in dbc files. The main value of this change IMO is just to "check the box": now it's done rather than TODO.

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.

4 participants