-
Notifications
You must be signed in to change notification settings - Fork 76
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
fix: update prov attributes combine #1116
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1116 +/- ##
===========================================
- Coverage 78.27% 58.30% -19.98%
===========================================
Files 65 13 -52
Lines 6265 1180 -5085
===========================================
- Hits 4904 688 -4216
+ Misses 1361 492 -869
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 54 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
The PR looks great, thanks! It does what it's supposed to do.
In test_combine_echodata_combined_append
I found a fair bit of code blocks (tests) that were nearly identical except and could be consolidated to use common code. So, I did that, plus a few other smaller improvements (IMHO) in readability, in this PR to your PR: lsetiawan#1
These changes are not needed, but I think they're helpful. Once you merge my PR (hopefully there won't be any problems), feel free to merge your PR.
Simplified by using common code for repeated blocks; plus other readability tweaks
@emiliom please approve this PR so I can merge it. Thanks! |
Overview
This PR address Issue #1115. Now the code checks and keep track of current attributes data from each combined files that exists. This probably can be optimized in the future, but for now it fixes the bug.