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

Add Spanish deck and ability to make a partial deck without specific card values #108

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

Conversation

dgarciah98
Copy link

@dgarciah98 dgarciah98 commented Nov 29, 2021

Added the ability to create a deck of a specific type (only have done it for the spanish deck, including pictures of all cards). This can be related to #6 since it mentions the german deck.
Also added the ability to create a partial deck but indicating which values (i.e.: 8s or 9s) you don't want in the deck, especially meant for the former feature.

Documentation has also been updated with these features.

@crobertsbmw
Copy link
Owner

Thanks for your help!
As far as the Spanish deck goes, is there any reason that there needs to be a separate CARDS definition? Or would it be acceptable to just have Spanish versions of the images? Or would it be acceptable to use all the sames card codes and just have a lang parameter to return different image files?
So https://deckofcardsapi.com/api/deck/a9g17zbmh81k/draw/?count=1&lang=es would return:

{
    "code": "AH", 
    "image": "https://deckofcardsapi.com/static/img/AE.png", 
    "value": "AS", 
    "suit": "ESPADAS"
}

Rather than having a new deck type field etc. This seems a lot simpler to me, with the added bonus of being able to switch languages mid game, or have a game where one player is English speaking, and another is Spanish speaking.

@dgarciah98
Copy link
Author

Thank you for the response!
While I know it may look cumbersome, I made the separate CARDS definition mainly for convenience, following the same logic with the card codes but for the Spanish cards, along with the pictures. Since I understood that the API responses are automatically formed depending on the card code found on the picture's name, I figured that it would be more easy this way, at least for myself.
On the other hand, in my opinion I found weird to have the same code as the English deck for the Spanish cards since neither they are the same nor mean the same, therefore I used different codes as a matter of consistency. Sure, I see why using the same codes for all cards is simpler, but again I find it pretty weird if you get a 3D code for the 3 of diamonds and then in the image you get a picture for the 3 of bastos, don't know if I'm making myself clear here...

My idea was also to have the ability to implement more kinds of decks (german, italian, tarot, w/e) in a more modular way in the future, thus having different card codes, separated pictures (within a directory of the same name as deck_type) and the deck_type field. I can understand that in the end this can be rather bloaty.

I think this and the former reasons respond to the CARDS definition and pictures for Spanish cards points.

As for the language matter that you mention, I didn't really think about it since I didn't imagine any English speaker using a Spanish deck for playing so I just wrote only the original names of the cards. English names could definitely be added.
But, on the other hand, this isn't really meant for switching languages or decks mid game. This was meant to be able to play a game of cards with a different deck that isn't the English using this API, especially games that were meant to play, in this case, with a Spanish deck (or in other words, card games played over here in Spain), like mus, cinquillo or brisca.
For example, cinquillo is meant to be played with a Spanish deck of 40 cards. While domino is similar to cinquillo, this game uses either 32 or 52 cards AFAIK, so it's not really compatible with other decks and of course it feels weird to play any of these games with an English deck.

I hope I covered everything and explained better why I did this regarding your questions, if not then I just filled the PR with a lot of text 😆

@crobertsbmw
Copy link
Owner

Sorry, I'm taking so long to respond. I see what you're saying. It does seem more congruent to have separate Spanish codes, I guess I'm just not sure if it's worth the bloat or not.
Hoping to find some time to review this a little more in depth today or tomorrow.

@dgarciah98
Copy link
Author

dgarciah98 commented Dec 3, 2021

No problem at all! No need to hurry.
I've thinking in the meantime some workarounds on how to palliate to some extent the bloat that this request implies.

First thing I thought of, while doesn't really reduce the bloat, at least it doesn't clutter the main code.
Rather than writing definitions (and the modifications to the response and to the functionality that have to be done) directly into models.py and views.py, I guess it could make some difference to write them into another file or class (let's say, models_custom.py and views_custom.py ) and call any of them from the main models class. Again, it doesn't really debloat, but it may make it somewhat cleaner maybe?

Next one was to, instead of manually writing all codes into a list, list all of them using listdir() and them take all the values into a list. This could actually work in general for any deck, even the default one, if all their respective pictures are inside a directory and therefore it would only depend on the deck_type field.

At some point I actually tried this while implementing it but didn't do it in the end since at the time I though this would sacrifice having the ability to create a new deck that is ordered since there isn't a proper way to sort the resulting list, or at least I didn't know how.
This was what I though of:

CARDS = [f.replace(".jpg","") for f in os.listdir("./static/img/"+deck_type)]
# deck_type = "spanish" in this case

Following this, in order to get that ordered list I recently thought of just simply adding the order into the pictures' filename,
i.e.: 01-AO.jpg, 02-2O.jpg, 03-3O.jpg ... 48-RB.jpg
Might seem dumb but I think it does the job. You would have to simply sort the list resulting from the previous line and also have to take out the first three characters to keep only the code but I think that can be easily done.

I hope this can bring some light into that issue.

@crobertsbmw
Copy link
Owner

Sorry I am so slow. I was looking at this again today.
I don't love the idea of reading the file names of the images, just because reading files from disc seems slow to me, especially if it's doing it on every request.
I'm coming around to the idea of deck_types. It could also allow modeling of other games, Uno, Rook, and not just other languages. I wonder how many people are already doing a re-mapping of their own to play other games that don't use a standard deck. I'm assuming that's why people requested the partial deck feature...

@dgarciah98
Copy link
Author

Definitely, deck_type could be used for those games. You could implement any kind of deck as long one provides, ideally, definitions and pictures, along with the modifications in the rest of the code, but I think this also implies the same issue that came up before if people were to provide them. Don't get me wrong, by no means I'm against it.

You are totally right on that it would have to read on every request, didn't really think about it. However I wonder if it has to read only every time /api/deck/new is called (which totally makes sense) or in every request. These are just my dumb thoughts and pretty sure it will be just the former, but I'll probably check it myself in the meantime since I don't have any more ideas of how to come around the bloat thing,
You may be also right on the "reading from disk seems slow" part, have to say it looked fast enough for me when I tried but it can be that I don't know what is "slow" and what is not for Python. As I said, I'll check it.

In any case, take your time.

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