Reset default chapter and verse at semicolons when parsing references #21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rences (20m) This could likely be considered a breaking change in how parsing works. Before, the only way to "reset" the default chapter was to encounter a reference with a colon in it. For example, "Genesis 2; 3:15" would parse correctly, but "Genesis 2; 3" would not, since the latter reference had no colon to mark it clearly as indicating a chapter. However, in common usage, the semicolon is what signals to the human reader that what is meant is not verse 3 of chapter 2, but chapters 2 and 3; this updates Pericope to behave the same way.
boblail
reviewed
Dec 21, 2019
| @@ -80,7 +80,8 @@ def match_all(text) | |||
| end | |||
|
|
|||
| def parse_reference(book, reference) | |||
| parse_ranges(book, normalize_reference(reference).split(/[,;]/)) | |||
| # Use positive lookahead to keep delimiter with the fragment that follows | |||
| parse_ranges(book, normalize_reference(reference).split(/(?=[,;])/)) | |||
There was a problem hiding this comment.
Just noting that the positive lookahead does impact performance — but not by a lot
Before
PERFORMANCE
0.021496 0.000342 0.021838 ( 0.021919)
0.191024 0.001824 0.192848 ( 0.193286)
1.908849 0.009966 1.918815 ( 1.924357)
After
PERFORMANCE
0.022117 0.000469 0.022586 ( 0.022739)
0.252090 0.004195 0.256285 ( 0.258896)
2.136289 0.012225 2.148514 ( 2.161139)
boblail
approved these changes
Dec 21, 2019
There was a problem hiding this comment.
The changes in the tests seem reasonable to me 👍
What do you think of making a corresponding PR on pericope.js?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This could likely be considered a breaking change in how parsing works. Before, the only way to "reset" the default chapter was to encounter a reference with a colon in it. For example, "Genesis 2; 3:15" would parse correctly, but "Genesis 2; 3" would not, since the latter reference had no colon to mark it clearly as indicating a chapter. However, in common usage, the semicolon is what signals to the human reader that what is meant is not verse 3 of chapter 2, but chapters 2 and 3; this updates Pericope to behave the same way.
This is a departure from previous behavior, as is illustrated by the test that is modified as part of this PR. Because of the use of commas and semicolons, the pair of references no longer parsed in the same way. (I referenced the comment and the expected ranges themselves to preference the way that seemed intended.) Even so, common usage being what it is, I would predict the impact of the change to be pretty minimal -- the handful of edge cases corrected by the change should outweigh the number of edge cases broken by the change. That said, I understand if this change needs further scrutiny and care because it changes behavior in this way.