-
Notifications
You must be signed in to change notification settings - Fork 97
Update block metadata once instead of four times #2453
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2453 +/- ##
=======================================
Coverage 91.09% 91.10%
=======================================
Files 465 465
Lines 39690 39696 +6
Branches 5312 5313 +1
=======================================
+ Hits 36155 36164 +9
- Misses 2020 2023 +3
+ Partials 1515 1509 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hannahbast
left a comment
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 looks good, thanks a lot. Minor suggestion + I will test this with qlever update-wikidata and come back to you
joka921
left a comment
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.
-
Concerning the code, I only have a small comment wording request.
-
Do I understand it correctly, that the old version was very safe (every function that changed anything about the located triples also updated the metadata), but that was inefficient because the outer interface then called this multiple times.
-
This then means that the new interface is more unsafe (because when working with the delta triples, you can now chain more functions without updating the metadata (which is only used outside the function), but tyhat is only called in very few places, and in the unit tests for the low level interfaces (where you can be expected to do lowlevel stuff).
|
@joka921 regarding 2. and 3. yes that's right. A compromise might be to do this with the snapshot in the |
RobinTF
left a comment
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.
Fine with the changes, I have an idea for a suggestion and a unit test likely needs adjustment, it's currently failing on clang 21.
|
@RobinTF Ready for another round. The test failure was random noise. The untested new lines are in an otherwise untested area. |
RobinTF
left a comment
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 one change seems unnecessarily complicated to me, other than that this seems good to go.
Overview
Conformance check passed ✅No test result changes. |
|
RobinTF
left a comment
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.
Thank you very much



Update the block metadata only once after an update. It is not needed during an update and was update 4 times before.