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

More resilient conversions from strings #6

Merged
merged 5 commits into from
Dec 19, 2023
Merged

Conversation

pashadia
Copy link
Contributor

  • Added case-insensitive conversions, so that e.g. Card::from("AH") works
  • Added clearer error messages when the conversion fails

Copy link
Owner

@Nydauron Nydauron left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This looks good for the most part.

Could you add some documentation to Suit::from_char explaining that the character is case-insensitive?

Also, CI failed to run due to clippy failing (assumably because some clippy rule was updated). I already went ahead and reordered the CI steps and fixed the clippy error on master. As I was writing this, I saw you made a push fixing it on this feature branch, and while I appreciate the change, I'd suggest popping off that commit and making that a separate PR.

@pashadia
Copy link
Contributor Author

Hey!

I've removed the CI-related commit, and I've added the documentation to the Suit::from_char() function.

As I've done that, I see that I've done the case-insensitive implementation for the Value conversion in the impl TryFrom<char> for Value block, instead of inside Value::from_char(). Functionally, it's the same thing, but I think these should have similar implementations -- would you like me to change the Value impl so that it's similar to the Suit ?

@pashadia
Copy link
Contributor Author

Rewritten the Value conversion from char in a similar fashion to Suit. Let me know if you have any comments.

Copy link
Owner

@Nydauron Nydauron left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Nydauron Nydauron merged commit 48c8502 into Nydauron:master Dec 19, 2023
1 check passed
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