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 pass event #492

Conversation

@danielimmke
Copy link
Contributor

danielimmke commented Oct 13, 2019

Checklist

  • Use a separate branch in your local repo (not master).
  • Test coverage is 100% (or you have a story for why it's ok).

For issue #477 - I did my best based on my understanding of the codebase. It appears to work well. I understand there's probably going to be some changes needed before it can be moved in but I am happy to make some adjustments if needed.

danielimmke added 2 commits Oct 13, 2019
Copy link
Owner

nicolodavis left a comment

Thanks for the PR!

src/core/flow.js Outdated Show resolved Hide resolved
src/core/flow.js Outdated Show resolved Hide resolved
@danielimmke

This comment has been minimized.

Copy link
Contributor Author

danielimmke commented Oct 14, 2019

Hey Nicolo, thank you for the very quick review time! I have a new commit with the changes you requested please let me know what you think.

@nicolodavis

This comment has been minimized.

Copy link
Owner

nicolodavis commented Oct 14, 2019

Looks good! Can you add some unit tests in flow.test.js demonstrating the usage of the Pass event?

danielimmke added 2 commits Oct 14, 2019
@danielimmke

This comment has been minimized.

Copy link
Contributor Author

danielimmke commented Oct 21, 2019

Sure thing, I will try to have that this week.

danielimmke added 2 commits Oct 27, 2019
@danielimmke danielimmke force-pushed the danielimmke:danielimmke/477-implement-a-pass-event branch from 14fcbbe to 26e408d Oct 30, 2019
@danielimmke danielimmke requested a review from nicolodavis Oct 30, 2019
@danielimmke

This comment has been minimized.

Copy link
Contributor Author

danielimmke commented Oct 30, 2019

Hey Nicolo,

So I basically copied over the tests for endTurn and removed some items to prevent redundancy (since pass calls the endTurn method under the hood and added a specific test for the remove parameter. Seems to work!

@danielimmke danielimmke force-pushed the danielimmke:danielimmke/477-implement-a-pass-event branch from 26e408d to 4abc7a0 Oct 30, 2019
src/core/flow.js Outdated Show resolved Hide resolved
@danielimmke danielimmke requested a review from nicolodavis Oct 30, 2019
@danielimmke

This comment has been minimized.

Copy link
Contributor Author

danielimmke commented Oct 30, 2019

Actually, I realized I'm not accounting for custom turn orders and more than 2 players. Please hold on reviewing for now. I will have another commit tonight/tomorrow.

danielimmke added 2 commits Oct 30, 2019
…dditional test to validate
…anielimmke/boardgame.io into danielimmke/477-implement-a-pass-event
@danielimmke

This comment has been minimized.

Copy link
Contributor Author

danielimmke commented Oct 30, 2019

Ok - I got that fixed. I think I broke my personal record for "most amount of time spent debugging for something that turned out to be a one line code change" - lol.

I also added an additional test to check that scenario.

danielimmke and others added 3 commits Nov 1, 2019
src/core/flow.js Outdated Show resolved Hide resolved
src/core/turn-order.js Outdated Show resolved Hide resolved
src/core/flow.js Outdated Show resolved Hide resolved
@danielimmke danielimmke requested a review from nicolodavis Nov 4, 2019
danielimmke and others added 2 commits Nov 8, 2019
@nicolodavis nicolodavis merged commit 1687ff8 into nicolodavis:master Nov 11, 2019
@nicolodavis

This comment has been minimized.

Copy link
Owner

nicolodavis commented Nov 11, 2019

Nice job @danielimmke!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.