-
-
Notifications
You must be signed in to change notification settings - Fork 517
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
Fix bug with option key ordering. #527
base: master
Are you sure you want to change the base?
Conversation
@DarryQueen I've merged your other PR [#528] but now this one has conflicts. Please resolve them and I'll merge. |
Done, merge conflicts resolved. |
import { optionsToString } from '../src/optionstostring' | ||
import { DateFormatter } from '../src/nlp/totext' | ||
import { datetime } from './lib/utils' | ||
|
||
const texts = [ | ||
['Every day', 'RRULE:FREQ=DAILY'], | ||
['Every day at 10, 12 and 17', 'RRULE:FREQ=DAILY;BYHOUR=10,12,17'], | ||
[ | ||
'Every week on Sunday at 10, 12 and 17', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this and moved it down in with "~ approximate" because it is technically not fully supported; byhour
is not fully convertible for weekly.
The logic for
isFullyConvertible
is sensitive to the key ordering oforigOptions
because it short-circuits once it sees one of the ignored keys. I imagine this is a bug; the ignored keys should just be skipped over in the loop.