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

Fix display impl for RRuleSet #100

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Fix display impl for RRuleSet #100

merged 1 commit into from
Apr 4, 2024

Conversation

fmeringdal
Copy link
Owner

No description provided.

@tomquist
Copy link
Contributor

Is there anything that's blocking this from being merged?

@fmeringdal
Copy link
Owner Author

fmeringdal commented Jan 21, 2024

This is just a partial fix that should fix most use-cases, so I have been hesitant about releasing this "fix" before I get time to fully fix it for all cases.

Have you been running into this bug?

@tomquist
Copy link
Contributor

Yeah, we’re migrating from rrule.js to the rust implementation using a napi wrapper and we’re running into test failures because the string representation didn’t contain all the things an RRuleSet from the JS lib contains.

What is missing in this PR for a full fix?

@fmeringdal
Copy link
Owner Author

Cool!

What is missing in this PR for a full fix?

The only thing should be if the rule uses one of the "VALUE=DATE" or "VALUE=PERIOD" properties where it will be interpreted as a date-time rather than just a date or a period.

Maybe you could try to run your test suite against this branch and see if there are other edge-cases not covered as well?

@fmeringdal fmeringdal force-pushed the fix/display-rruleset branch from 416ee79 to 0d74f05 Compare April 4, 2024 14:02
@fmeringdal fmeringdal merged commit 2dffa94 into main Apr 4, 2024
8 checks passed
@fmeringdal fmeringdal deleted the fix/display-rruleset branch April 4, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants