-
Notifications
You must be signed in to change notification settings - Fork 29
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
Validate Repo Integrity #173
Conversation
Codecov Report
@@ Coverage Diff @@
## master #173 +/- ##
==========================================
+ Coverage 95.22% 95.39% +0.18%
==========================================
Files 66 69 +3
Lines 11881 12268 +387
Branches 1011 1067 +56
==========================================
+ Hits 11313 11703 +390
+ Misses 371 365 -6
- Partials 197 200 +3
|
@hhsecond can you take a look at this? The tests aren't complete, it needs documentation, and there are a few corner cases to take care of, but I'd like your opinion on if I'm missing anything significant. My TODO list right now is:
Also, i'm not sure if I want to be raising a
HOWEVER, raising errors in this fashion means that it's not really possible to generate a list of issues should there be multiple things wrong . In addition to stopping at the first error it encounters, this doesn't return a machine readable description of the issue. In its current form, there's not any reasonable path towards effective logging or automated attempts to recover data. These seem like issues for the future, but I'm not sure if you have other thoughts? |
58dc2cc
to
711899d
Compare
TODOS complete and tests finished. When you have a chance, would love to hear your thoughts @hhsecond. No rush. |
711899d
to
1a1a1fc
Compare
This pull request introduces 4 alerts when merging 68fbe4b into d267c0a - view on LGTM.com new alerts:
|
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.
I had run through the code and found super minor tweaks we could do. I'll try to finish it before EOD
recs = self._traverse_all_hash_records() | ||
out = list(map(parsing.hash_data_raw_key_from_db_key, recs)) | ||
recs = self._traverse_all_hash_records(keys=True, values=False) | ||
out = list(map(hash_data_raw_key_from_db_key, recs)) |
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.
list comprehension? Here and few other places where we have map -> list conversion
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.
list comprehension is ~10% slower than mapping when the function is predefined. I've changed it where appropriate, but won't be making the change universally.
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.
I think
list(map(somefun, recs))
> [somefun(val) for val in recs]
> map(somefun, recs)
I mean, it's probably negligible but just saying if you want to do a comparison
This pull request fixes 3 alerts when merging e38844f into d267c0a - view on LGTM.com fixed alerts:
|
…ification chain. Also including last funcs params so that we can identify which particular record is the issue
e38844f
to
98123e9
Compare
This pull request fixes 3 alerts when merging 98123e9 into d267c0a - view on LGTM.com fixed alerts:
|
Motivation and Context
Why is this change required? What problem does it solve?:
Validate the integrity of repo data and commit history.
Description
Describe your changes in detail:
Types of changes
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Is this PR ready for review, or a work in progress?
How Has This Been Tested?
Put an
x
in the boxes that apply:Checklist: