[feat] merge relative date refiner #430
Conversation
|
Hey @liamcain. Thank you for your work! |
|
Hi @wanasit, is there a review you forgot to submit? I don't see the comment you're referring to. Also just noticed that I had |
|
|
||
| shouldMergeResults(textBetween: string, currentResult: ParsingResult, nextResult: ParsingResult): boolean { | ||
| return ( | ||
| textBetween.match(this.patternBetween()) != null && |
There was a problem hiding this comment.
This is difficult to understand. Could we break this into multiple if-return-false? e.g.
if (<not match>) {
return false;
}
if (<not time ago>) {
return false;
}
...
return true;
There was a problem hiding this comment.
Great point, this was confusing for me to read just a few days after having written it. Improved the code style here and added some more context to make it easier follow.
| * - 2 weeks before 2020-02-13 | ||
| * - 2 days after next Friday | ||
| */ | ||
| export default class ENMergeDateRelativeRefiner extends MergingRefiner { |
There was a problem hiding this comment.
Nit: I think ENMergeRelativeDateRefiner is more accurate and consistent with the other merge refiners
There was a problem hiding this comment.
Changed to your suggest. I was also thinking ENMergeImpliedReferenceDateRefiner might be more accurate, lmk what you think.
| const PATTERN = new RegExp(`(${TIME_UNITS_PATTERN})\\s{0,5}(?:ago|before|earlier)(?=(?:\\W|$))`, "i"); | ||
| const STRICT_PATTERN = new RegExp(`(${TIME_UNITS_PATTERN})\\s{0,5}ago(?=(?:\\W|$))`, "i"); | ||
| export const PATTERN = new RegExp(`(${TIME_UNITS_PATTERN})\\s{0,5}(?:ago|before|earlier)(?=(?:\\W|$))`, "i"); | ||
| export const STRICT_PATTERN = new RegExp(`(${TIME_UNITS_PATTERN})\\s{0,5}ago(?=(?:\\W|$))`, "i"); |
There was a problem hiding this comment.
Do we need to export both strict and normal? could you export only the one we need?
There was a problem hiding this comment.
Reverted. I mostly did this out of laziness, but I agree that having more precise pattern matching here is better.
Ops. Sorry, I'm actually not very used to CR through Github ^^; |
|
@wanasit Thanks for the review! I addressed all your comments |
Summary
Adds support for more complex composite dates.