Closed thombruce closed 1 month ago
netlify[bot]
commented
1 month ago | Name | Link |
|---|---|
| Latest commit | d0d07f514f418ec9bcf6ddcf5d39da668a2da7ad |
| Latest deploy log | https://app.netlify.com/sites/toodles/deploys/66bc1feaa5b001000895cab9 |
github-actions[bot]
commented
1 month ago | Status | Category | Percentage | Covered / Total |
|---|---|---|---|
| 🟢 | Lines | 69.71% / 60% | 435 / 624 |
| 🟢 | Statements | 69.71% / 60% | 435 / 624 |
| 🟢 | Functions | 65.85% / 60% | 27 / 41 |
| 🟢 | Branches | 72.05% / 60% | 49 / 68 |
thombruce
commented
1 month ago The code has been heavily refactored on the fly as part of the transition. I've tested some of the behaviours and have some confidence it's all working, but I haven't fully reviewed my efforts yet.
Notably, all if else weekday testers have been replaces with a single line:
else if (days = str.match(/((?:mon|tues|wednes|thurs|fri|satur|sun)day)/gi)) return Schedule.days(days.map(d => dayToWeekday(d)))I wasn't completely happy that that regex had to abandon start and end of string anchors (^$) but I was struggling to get the expected behaviour out of it while they were present. This will now match any weekday, if one is present, anywhere in the every string. That... should be fine, honestly. There is currently no other reason to include weekdays by name except for this case.
dayToWeekday() is a custom function that matches the weekday string to its equivalent RRule designation:
function dayToWeekday(day) {
if (/^monday$/i.test(day)) return RRule.MO
else if (/^tuesday$/i.test(day)) return RRule.TU
else if (/^wednesday$/i.test(day)) return RRule.WE
else if (/^thursday$/i.test(day)) return RRule.TH
else if (/^friday$/i.test(day)) return RRule.FR
else if (/^saturday$/i.test(day)) return RRule.SA
else if (/^sunday$/i.test(day)) return RRule.SU
}One obvious revision might be to permit partial day strings to reflect common weekday abbreviations:
An example regex for Thursdays: /thu?r?s?(?:day)?/ <- _Though, that would also matchthrdayandthsday. Any chance we can be more robust? Ensure that each previous character does actually exist? Maybe like..._ ->/(?:th|thu|thur|thurs|thursday)/` This becomes very verbose when we add all the other weekdays. Something to work on, not urgently.
thombruce
commented
1 month ago If we do start matching abbreviations in our weekday match, then when it comes to pairing that with its RRule counterpart we can actually discard most of the string. We only need the first two characters:
function dayToWeekday(day) {
if (/^mo/i.test(day)) return RRule.MO
else if (/^tu/i.test(day)) return RRule.TU
else if (/^we/i.test(day)) return RRule.WE
else if (/^th/i.test(day)) return RRule.TH
else if (/^fr/i.test(day)) return RRule.FR
else if (/^sa/i.test(day)) return RRule.SA
else if (/^su/i.test(day)) return RRule.SU
}We should probably still use regexes so that we can maintain case insensitivity. There is another way where we cast the string to a lowercase string and check whether it startsWith but my focus is on keeping this code as lean as possible. I don't actually know what method would perform best, but performance can be a later concern.
thombruce
commented
1 month ago So how's this?
else if (days = str.match(/((?:mon?|tue?s?|wed?(?:nes)?|thu?r?s?|fri?|sat(?:ur)?|sun?)(?:day)?)/gi)) return Schedule.days(days.map(d => dayToWeekday(d)))It's gonna have some false positives, for sure. Malformed weekday strings, like thrday, are going to be matched... but that's actually not a huge problem, right? Our main concern are those first two characters - so long as those are correct, it doesn't matter!
Ah, no! There is a genuine issue with this...
Since I've made a lot of parts optional and we don't have anchors in this regex, it's going to match partial values anywhere in the string like... "mo"... which will match "monthly" <- That's a false positive we do care about!
Moves schedule logic out of Todo scope and into own module.
If applicable, we may migrate this code to TNT Core.
By submitting this pull request, you agree to follow our Code of Conduct: https://github.com/thombruce/.github/blob/main/CODE_OF_CONDUCT.md
Internal use. Do not delete.