Skip to content
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

feat(plugins/dates): support duration ranges #1137

Merged
merged 5 commits into from
Aug 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions plugins/dates/src/api/parse/range/02-date-range.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import parseDate from '../one/index.js'
import reverseMaybe from './_reverse.js'
import Unit from '../one/units/Unit.js'

export default [
{
Expand Down Expand Up @@ -138,4 +139,56 @@ export default [
return null
},
},

{
// in 2 to 4 weeks
match: '^in [<min>#Value] to [<max>#Value] [<unit>(day|days|week|weeks|month|months|year|years)]',
desc: 'in 2 to 4 weeks',
parse: (m, context) => {
const { min, max, unit } = m.groups()

let start = new Unit(context.today, null, context)
let end = start.clone()

const duration = unit.text('implicit')
start = start.applyShift({ [duration]: min.numbers().get()[0] })
end = end.applyShift({ [duration]: max.numbers().get()[0] })

// Ensure that the end date is inclusive
if (!['day', 'days'].includes(duration)) {
end = end.applyShift({ day: -1 }).applyShift({ [duration]: 1 })
}
Comment on lines +158 to +160
Copy link
Contributor Author

@Fdawgs Fdawgs Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully this is an okay way of doing this.
I found that days was projecting inclusively, but, other durations weren't.

For example 1-2 weeks was returned as 8 days rather than 14 days.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks @spencermountain!
Hopefully last question, do i need to do anything extra for handling past tense in this?
I.e. 2 to 3 weeks ago.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, no problem - i would make a new section, in addition to this '2 to 4 weeks' one, with similar logic.
go for it!

oh, one thing - maybe add a '^in' on the match, so it doesn't interfere with 2 to 3 weeks after june 3rd etc.
lookin good! thanks for the help.

Copy link
Contributor Author

@Fdawgs Fdawgs Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers @spencermountain, have added the ^in as requested for the future tense match block.

Struggling with the past tense, as you can see from the now failing tests! When you get a spare second could you take a look at the past tense block please? Thought it'd be as simple as flipping the logic.
If it's not a simple fix, happy to just tear out past tense support for now.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure - i can merge this to dev and fix the failing tests, if you're ready?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey Frazer, haven't forgotten about this - been really busy, then had covid. Will try to get to it shortly.
cheers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on @spencermountain, 42f9cd0 appears to have stopped the ranges from being inclusive?


return {
start: start,
end: end.end(),
}
},
},
{
// 2 to 4 weeks ago
match:
'[<min>#Value] to [<max>#Value] [<unit>(day|days|week|weeks|month|months|year|years)] (ago|before|earlier|prior)',
desc: '2 to 4 weeks ago',
parse: (m, context) => {
const { min, max, unit } = m.groups()

let start = new Unit(context.today, null, context)
let end = start.clone()

const duration = unit.text('implicit')
start = start.applyShift({ [duration]: -max.numbers().get()[0] })
end = end.applyShift({ [duration]: -min.numbers().get()[0] })

// Ensure that the end date is inclusive
if (!['day', 'days'].includes(duration)) {
end = end.applyShift({ day: 1 }).applyShift({ [duration]: -1 })
}

return {
start: start,
end: end.end(),
}
},
},
]
65 changes: 65 additions & 0 deletions plugins/dates/tests/duration-range.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import test from 'tape'
import nlp from './_lib.js'

// Duration ranges
const durArr = [
{
text: ['2 to 4 days', '2-4 days', 'two to four days'],
duration: { years: 0, months: 0, days: 3, hours: 0, minutes: 0 },
},
{
text: ['1 to 2 weeks', '1-2 weeks', 'one to two weeks'],
duration: { years: 0, months: 0, days: 14, hours: 0, minutes: 0 },
},
{
text: ['1 to 5 months', '1-5 months', 'one to five months'],
duration: { years: 0, months: 5, days: 0, hours: 0, minutes: 0 },
},
{
text: ['2 to 4 years', '2-4 years', 'two to four years'],
duration: { years: 3, months: 0, days: 0, hours: 0, minutes: 0 },
},
]

const context = {
today: '2024-01-01',
}

test('future duration-ranges', function (t) {
durArr.forEach(obj => {
obj.text.forEach(text => {
const doc = nlp(`in ${text}`)
const { duration, start, end } = doc.dates(context).get()[0]
t.deepEqual(duration, obj.duration, text)
t.ok(start > context.today, 'start date')
t.ok(end > start, 'end date')
})
})
t.end()
})

test('past duration-ranges', function (t) {
durArr.forEach(obj => {
obj.text.forEach(text => {
const doc = nlp(`${text} ago`)
const { duration, start, end } = doc.dates(context).get()[0]
t.deepEqual(duration, obj.duration, text)
t.ok(start < context.today, 'start date')
t.ok(end > start, 'end date')
})
})
t.end()
})

test('past duration-ranges', function (t) {
durArr.forEach(obj => {
obj.text.forEach(text => {
const doc = nlp(`${text} ago`)
const { duration, start, end } = doc.dates(context).get()[0]
t.deepEqual(duration, obj.duration, text)
t.ok(start < context.today, 'start date')
t.ok(end > start, 'end date')
})
})
t.end()
})
Loading