Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #237 +/- ##
==========================================
- Coverage 90.92% 90.91% -0.01%
==========================================
Files 337 337
Lines 21542 21642 +100
==========================================
+ Hits 19586 19675 +89
- Misses 1956 1967 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 8 of 10 files at r1, all commit messages.
Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion
machine/corpora/usfm_update_block_handler.py line 11 at r1 (raw file):
class UsfmUpdateBlockHandlerException(Exception):
You should name this UsfmUpdateBlockHandlerError so that it is consistent with Python naming conventions.
Enkidu93
left a comment
There was a problem hiding this comment.
Reviewable status: 4 of 10 files reviewed, 1 unresolved discussion (waiting on @ddaspit)
machine/corpora/usfm_update_block_handler.py line 11 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You should name this
UsfmUpdateBlockHandlerErrorso that it is consistent with Python naming conventions.
I wasn't sure about this. Done.
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 2 of 10 files at r1.
Reviewable status: 6 of 10 files reviewed, 2 unresolved discussions (waiting on @Enkidu93)
machine/corpora/scripture_ref.py line 125 at r1 (raw file):
# Using to_relaxed() is necessary to maintain equality across relaxed refs, # __eq__ properly handles relaxed ref comparison return hash((self.verse_ref, tuple(self.to_relaxed().path)))
This is different than the implementation in Machine. On the other hand, Machine is inconsistent with itself, which is probably not a good thing. ScriptureRefComparer.GetHashCode() and ScriptureRef.GetHashCode() give different results, which would be unexpected for a caller. Why do we need to call to_relaxed here?
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)
That's a great question. Maybe the simplest thing to do is to just remove the scripture ref comparer since it's not being used with the current row-lookup solution. The reason that I used |
ddaspit
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)
machine/corpora/scripture_ref.py line 125 at r1 (raw file):
Previously, Enkidu93 (Eli C. Lowry) wrote…
That's a great question. Maybe the simplest thing to do is to just remove the scripture ref comparer since it's not being used with the current row-lookup solution. The reason that I used
to_relaxed()is because of the intransitivity ofScriptureRefequality when you have, say, one relaxed ref and two non-relaxed refs with the same markers. If we want relaxed and non-relaxed refs to fall into the same bucket properly, we would need to fall back on__eq__which handles this correctly. But as I think about this more, I'm not even sure that that would work - it would depend on how Python implementsdict. What do you think?
If we are not using it, let's go ahead and remove it.
Enkidu93
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)
machine/corpora/scripture_ref.py line 125 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
If we are not using it, let's go ahead and remove it.
Done.
Enkidu93
left a comment
There was a problem hiding this comment.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @ddaspit)
machine/corpora/scripture_ref.py line 125 at r1 (raw file):
Previously, Enkidu93 (Eli C. Lowry) wrote…
Done.
Removing it in Machine here as well
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)
Port
Sorry this took so long - I hit a bug that I just couldn't figure out since I was certain I had ported the relevant code correctly. Turns out that machine.py and Machine code had become slightly out of sync. I went ahead and made the change here to make it consistent with Machine, but I'm suspicious that Machine was actually the incorrect one/missing a commit. I think that maybe #170 never got entirely properly incorporated into Machine because it went in around the same time as the update block code. Could you take a peek, @ddaspit, and let me know what's right and what's wrong? Also happy to just push this through since the tests are passing and follow up on this in a separate issue.
This change is