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

Decode: don't panic when date time is missing timezone #614

Merged
merged 4 commits into from
Oct 7, 2021

Conversation

RiyaJohn
Copy link
Contributor

@RiyaJohn RiyaJohn commented Oct 4, 2021

Issue: #596

Explanation of what this pull request does.
Fixes the issue of throwing unexpected panic
Returns more meaningful error for the user

More detailed description of the decisions being made and the reasons why (if the patch is non-trivial).

@RiyaJohn RiyaJohn changed the base branch from master to v2 October 4, 2021 08:40
Copy link
Owner

@pelletier pelletier left a comment

Choose a reason for hiding this comment

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

Thank you for submitting a patch! We purposefully do not use regexps in this library because they are redundant (we're already writing a validating state machine), slower, and incurs memory allocation. We need a different solution for this problem.

@RiyaJohn
Copy link
Contributor Author

RiyaJohn commented Oct 5, 2021

Thank you for submitting a patch! We purposefully do not use regexps in this library because they are redundant (we're already writing a validating state machine), slower, and incurs memory allocation. We need a different solution for this problem.

Got it, will work on a different solution

@RiyaJohn
Copy link
Contributor Author

RiyaJohn commented Oct 6, 2021

@pelletier Made changes to not use regexp

@pelletier pelletier merged commit e967463 into pelletier:v2 Oct 7, 2021
@pelletier
Copy link
Owner

Thanks for the patch! Removed a debug statement and added a test to double check the reported issue was fixed.

@RiyaJohn
Copy link
Contributor Author

RiyaJohn commented Oct 7, 2021

Thanks @pelletier, will be great if you could add the hactoberfest-accepted label just in case hactoberfest topic is not there for repo

@pelletier
Copy link
Owner

@RiyaJohn added, but there is the hacktoberfest topic on the repo.

@jidicula jidicula mentioned this pull request Oct 15, 2021
@pelletier pelletier added the bug Issues describing a bug in go-toml. label Oct 28, 2021
@pelletier pelletier changed the title Fixes #596: Panic date time should have a timezone Decode: don't panic when date time is missing timezone Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues describing a bug in go-toml. hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants