Skip to content

feat: introduce flag validate_user_defined_fields - #352

Open
Vlad-Bukatin wants to merge 4 commits into
Validus-Risk-Management:mainfrom
Boku-Labs:ignore_user_defined_fields
Open

feat: introduce flag validate_user_defined_fields#352
Vlad-Bukatin wants to merge 4 commits into
Validus-Risk-Management:mainfrom
Boku-Labs:ignore_user_defined_fields

Conversation

@Vlad-Bukatin

@Vlad-BukatinVlad-Bukatin commented May 14, 2026

Copy link
Copy Markdown
Contributor

closes: #331

@codecov

codecovBot commented May 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.67593% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.84%. Comparing base (b8cb424) to head (f8e9b92).

Files with missing linesPatch %Lines
crates/hotfix-message/src/builder.rs93.58%20 Missing ⚠️
crates/hotfix/src/session.rs83.33%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #352 +/- ##
==========================================
+ Coverage 91.72% 91.84% +0.12% 
==========================================
Files 94 94 Lines 10038 10423 +385 ==========================================
+ Hits 9207 9573 +366 - Misses 831 850 +19 
FlagCoverage Δ
core91.75% <94.67%> (+0.12%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Vlad-Bukatin
Vlad-Bukatinforce-pushed the ignore_user_defined_fields branch from 579d0cf to 0b8f197CompareMay 14, 2026 07:59
@davidsteiner

Copy link
Copy Markdown
Member

@Vlad-Bukatin sorry, I'll get to this as soon as I can. I just have quite a lot going on, and this change is a bit larger so I want to give it a proper read.

@davidsteinerdavidsteiner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The general direction looks good to me. My only functional concern is how we check the parent groups - left a comment on that.

Also, we're not too strict on test coverage, but I feel like builder.rs has a few interesting branches which aren't covered by any tests. It would be great to add some coverage for those.

Apologies for the very slow review.

Comment threadcrates/hotfix-message/src/builder.rs Outdated
.next_field()
.ok_or(ParserError::Malformed("incomplete group".to_string()))?
};
} else if Self::parent_contains_tag(parent, current_tag)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I read this correctly, this checks only the immediate parent. I think this is an issue for any scenario where the next tag isn't one for the enclosing group, but another ancestor - e.g. when an inner group and an outer group end at the same tag, so the next tag is for the message (the outer group's parent). What do you think?

@Vlad-Bukatin

Vlad-Bukatin commented Jun 15, 2026

Copy link
Copy Markdown
ContributorAuthor

@davidsteiner Thanks for the review! No worries at all about the delay - I've been quite busy on my end as well.
I'll look into your comment regarding the parent groups logic and update that. I will also add some test coverage (was planning for some time... but didn't have a chance to look into it).

Hopefully I would be able to do it next week

@Vlad-Bukatin

Copy link
Copy Markdown
ContributorAuthor

@davidsteiner took me some time, but I think it is ready

Sign up for freeto 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.

Feature Request: Add configuration to allow unknown/custom XML fields

2 participants

@Vlad-Bukatin@davidsteiner