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

Standardize Card to String conversion to use UTF-8 suit symbols #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pashadia
Copy link
Contributor

This will allow the Cards to be parsed back from their .to_string() output.

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.

I like the addition.

The one thing I didn't realize about Display is that it implicitly implements the trait ToString. So, there are two different string conversion methods, the other one being:

impl From<Card> for String {
fn from(c: Card) -> Self {
format!("{}{}", c.value.get_char(), c.suit.get_char())
}
}

which prints out an alpha character for the suit rather than a symbol. My intentions with Display were to provide a prettified way of displaying the cards while also giving another the option to convert the Card (unaware of ToString). Seeing that there doesn't seem to be a way to override the blanket implementation, I guess it is best to also change From<Card> to just use to_string() (same inconsistency issue applies to From<Suit> for char).

If those changes could be made, alongside adding some additional documentation to Suit::from_char() referencing the ability to parse symbols, I think the PR would be good to merge.

@Nydauron Nydauron changed the title Read from UTF-8 suit symbols Standardize Card to String conversion to use UTF-8 suit symbols Dec 23, 2023
@pashadia
Copy link
Contributor Author

pashadia commented Dec 23, 2023

I've made those changes, with the following exception:

same inconsistency issue applies to From<Suit> for char

That's incorrect. .to_string() returns a String not a char.

@Nydauron
Copy link
Owner

The inconsistency I was referring to was that suit.into::<char>() currently returns back an alpha character (e.g. 'h') while suit.to_string() returns a symbol string (e.g. "♥"). Ideally these should be the same character (the return types still being a char and a String respectively):

let suit = Suit::Heart;
assert_eq!(suit.into::<char>(), '♥');
assert_eq!(suit.to_string(), "♥");

@pashadia
Copy link
Contributor Author

Ok, that's doable, but are you sure you want that?

I think that having some way in which the structs would return an alphanumeric char/string could be beneficial for testing and rapid prototyping purposes.

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