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 calendar parsing on from_str implementation #191

Merged
merged 2 commits into from
Feb 12, 2025
Merged

Conversation

nekevss
Copy link
Member

@nekevss nekevss commented Feb 11, 2025

parse_allowed_calendar_formats was returning Some(None) instead of None. This was preventing calendar ids from correctly moving to Calendar::from_utf8

@nekevss nekevss added the C-bug Something isn't working label Feb 11, 2025
@jedel1043
Copy link
Member

Uhhh can you remind me again why we have Option<Option<&[u8]>>? There are too many empty values there:

  • None
  • Some(None)
  • Some(Some(&[]))

Do we use all of them?

@nekevss
Copy link
Member Author

nekevss commented Feb 11, 2025

Hmmmm, we're swallowing the error here from the ParseCalendarString.

So the return type could be Result<Option<&[u8]>, but we are swallowing the error anyways in this instance, so it doesn't offer much utility to return an Error. As it currently stands, the Some(None) return was incorrectly signalling a normal completion on the format parsing and going down the wrong code path.

EDIT: The question on Some(Some(&[])) is interesting. I believe those are rejected by Calendar::from_utf8 (Plus I believe [u-ca=] should be an invalid annotation)

@nekevss
Copy link
Member Author

nekevss commented Feb 12, 2025

Update: I was able to confirm the ixdtf does correctly throw an error on [u-ca=], so the empty value case of Some(Some(&[])) is handled during parsing.

Even on the cases that it is not handled, it would be rejected in Calendar::from_utf8.

@jedel1043
Copy link
Member

jedel1043 commented Feb 12, 2025

@nekevss In that case, can we flatten the Options and use &[] as our empty value instead?

@nekevss
Copy link
Member Author

nekevss commented Feb 12, 2025

can we flatten the Options and use &[] as our empty value instead?

Sure! Actually, I think I like this better. Added a sanity test too for empty calendar annotations.

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Looks good!

@nekevss nekevss merged commit 6a3c74f into main Feb 12, 2025
7 checks passed
@jedel1043 jedel1043 deleted the fix-calendar-parsing branch February 12, 2025 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants