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.

Sign up for GitHub

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

Jump to bottom

add game: crazy eights #984

Closed
wants to merge 4 commits into from
Closed

add game: crazy eights #984

wants to merge 4 commits into from

Conversation

rezunli96
Copy link
Contributor

No description provided.

@lanctot
Copy link
Collaborator

lanctot commented Dec 31, 2022

Thanks!!

Please note that the github tests are currently broken but will get fixed once #983 is merged, hopefully in the next week or so.

@lanctot
Copy link
Collaborator

lanctot commented Jan 2, 2023

Ok #983 has been merged. Can you pull changes from master and commit the merge to retrigger the tests?

@lanctot lanctot requested a review from jhtschultz January 9, 2023 15:46
@lanctot lanctot assigned jhtschultz and unassigned jhtschultz Jan 9, 2023
jhtschultz
jhtschultz reviewed Jan 13, 2023
View reviewed changes
Copy link
Member

@jhtschultz jhtschultz left a comment

Choose a reason for hiding this comment

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

Classic game and definitely one that belongs in Open Spiel, thanks for the contribution!

One high level suggestion, please run cpplint (https://github.com/cpplint/cpplint) so that everything conforms to the Google C++ style guide.

docs/games.md Outdated Show resolved Hide resolved
open_spiel/games/crazy_eights.h Outdated Show resolved Hide resolved
open_spiel/games/crazy_eights.h Outdated Show resolved Hide resolved
open_spiel/games/crazy_eights.h Outdated Show resolved Hide resolved
open_spiel/games/crazy_eights.h Outdated Show resolved Hide resolved
open_spiel/games/crazy_eights.cc Outdated Show resolved Hide resolved
open_spiel/games/crazy_eights.cc Outdated Show resolved Hide resolved
open_spiel/games/crazy_eights.cc Outdated Show resolved Hide resolved
open_spiel/games/crazy_eights.h Outdated Show resolved Hide resolved
open_spiel/games/crazy_eights_test.cc Outdated Show resolved Hide resolved
@rezunli96
Copy link
Contributor Author

@jhtschultz thanks for the review! please take a look at my revision. I reformatted it according to Google style

@elkhrt elkhrt added the imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! label Feb 7, 2023
@jhtschultz
Copy link
Member

Sorry for the delay. Thanks for the updates, LGTM!

@rezunli96
Copy link
Contributor Author

Thanks! And I just fixed another bug that corrects the action ID. Please see the newest commit.

OpenSpiel pushed a commit that referenced this pull request Feb 20, 2023
@lanctot
Merge pull request #984 from rezunli96:crazy_eights
e0c3e9a 
PiperOrigin-RevId: 507779143
Change-Id: I0e4375ca80b41d0d31ff24bdd7a00735caf42870
@lanctot
Copy link
Collaborator

lanctot commented Feb 20, 2023

Thanks! And I just fixed another bug that corrects the action ID. Please see the newest commit.

I don't think this made it in because Ed imported before the new commit. Can you check the commit logs, and submit another PR if necessary?

@lanctot
Copy link
Collaborator

lanctot commented Feb 20, 2023

(Also you can close this one now as it's been merged.)

@rezunli96 rezunli96 closed this Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhtschultz jhtschultz jhtschultz left review comments

@lanctot lanctot lanctot left review comments

Assignees

@jhtschultz jhtschultz

Labels
imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rezunli96 @lanctot @jhtschultz @elkhrt