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

Close #121 #122

Closed
wants to merge 2 commits into from
Closed

Close #121 #122

wants to merge 2 commits into from

Conversation

laralove143
Copy link
Contributor

No description provided.

@djc
Copy link
Member

djc commented Dec 30, 2022

This mostly looks good. Can you please give the existing commit a meaningful commit message, and add a comment that bumps the version number for chrono-tz because this is a semver-incompatible change?

@laralove143
Copy link
Contributor Author

I don't know how to edit a commit message after a push

@djc
Copy link
Member

djc commented Dec 30, 2022

Well, you could have googled it. Try git rebase -i main and put an r (for reword) next to both of your commits. It should bring up your editor for both commits, move the commit message on the second commit to the first commit and change the second commit to something like "Bump version for semver-incompatible changes".

@laralove143 laralove143 closed this by deleting the head repository Jan 28, 2023
@pitdicker
Copy link
Contributor

I'd like to reopen this PR. It was closed because the author deleted his fork of the repo.

But I'm not a fan of enums that change depending on the crate features. And we should especially not remove the Copy implementation when the std feature is enabled.

@djc Does it give any benefit to include the string that failed to parse? It is passed to from_str by reference so the user can easily wrap it with something like anyhows with_context.

@djc
Copy link
Member

djc commented Apr 4, 2024

Leaving the string out of seems reasonable.

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.

3 participants