New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move structure validation to separate structure validator #5939
Move structure validation to separate structure validator #5939
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5939 +/- ##
==========================================
- Coverage 26.81% 26.74% -0.08%
==========================================
Files 376 377 +1
Lines 47777 47718 -59
==========================================
- Hits 12813 12763 -50
+ Misses 34964 34955 -9 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self review.
Ok(()) | ||
} | ||
|
||
pub(crate) fn validate_feature_def(&self, feature_def: &FeatureDef) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is the largest non-mechanical change: here we're aggregating a lot of disparate methods which were all iterating over feature_def.props
, and put them all in one loop.
)], | ||
false, | ||
); | ||
validator.validate_feature_def(&fm).expect_err( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these tests used to be of the form:
let fm = get_feature_manifest(…)
fm.validate_something()
I've left most of the variable names intact.
9740f54
to
5407918
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! 👏
5407918
to
3efedc8
Compare
Relates to EXP-3938.
According to The Big O of Code Reviews, this is a O(n) change.
This is moving quite a lot of code out
intermediate_representation.rs
into a newstructure/validator.rs
file.The changes are fairly mechanical, but they added up. The tests also changed and moved, so this is a larger O(n) sized.
This has a requirement on landing #5947 and rebasing before this will build.
Pull Request checklist
[ci full]
to the PR title.Branch builds: add
[firefox-android: branch-name]
to the PR title.