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

TypeScript UNO #4825

Merged
merged 4 commits into from
Sep 1, 2018
Merged

TypeScript UNO #4825

merged 4 commits into from
Sep 1, 2018

Conversation

KrisXV
Copy link
Member

@KrisXV KrisXV commented Aug 24, 2018

No description provided.

@@ -29,6 +29,14 @@ const textColors = {

const textShadow = 'text-shadow: 1px 0px black, -1px 0px black, 0px -1px black, 0px 1px black, 2px -2px black;';

/** @typedef {'Green' | 'Yellow' | 'Red' | 'Blue' | 'Black'} Color */
/** @typedef {{value: string, color: Color | string, changedColor?: Color, name: string}} Card */
Copy link
Member

Choose a reason for hiding this comment

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

I assume color should always be a color as defined above and not any string.

class UnoGame extends Rooms.RoomGame {
/**
* @param {ChatRoom} room
* @param {number | string} cap
Copy link
Member

Choose a reason for hiding this comment

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

Would probably be best to just support a string or number, not both.

this.playerCap = cap;
/** @type {boolean} */
Copy link
Member

Choose a reason for hiding this comment

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

Already supported in Rooms#RoomGame

this.gameid = 'uno';
/** @type {string} */
Copy link
Member

Choose a reason for hiding this comment

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

Both of these are already supported in Rooms#RoomGame, no need to re-type them

@@ -163,39 +217,47 @@ class UNOgame extends Rooms.RoomGame {
this.players[user.userid].userid = user.userid;
if (this.awaitUno && this.awaitUno === oldUserid) this.awaitUno = user.userid;

if (this.currentPlayer && this.currentPlayer === oldUserid) this.currentPlayer = user.userid;
if (this.currentPlayerid && this.currentPlayerid === oldUserid) this.currentPlayerid = user.userid;
Copy link
Member

Choose a reason for hiding this comment

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

You shouldnt need to check if currentPlayerid is falsey, they should both be strings and a simple compare check should work (which your already doing).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this was when currentPlayer still defaulted to null and i forgot to remove null checks

@@ -394,31 +487,47 @@ class UNOgame extends Rooms.RoomGame {
this.nextTurn();
}

/**
* @param {UnoGamePlayer} user
* @param {number | string} count
Copy link
Member

Choose a reason for hiding this comment

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

Id try to handle only one type


let player = this.players[user.userid];
player.hand.push(...drawnCards);
player.sendRoom(`|raw|You have drawn the following card${Chat.plural(drawnCards)}: ${drawnCards.map(card => `<span style="color: ${textColors[card.color]}">${card.name}</span>`).join(', ')}.`);
return drawnCards;
}

/**
* @param {string | number} count
Copy link
Member

Choose a reason for hiding this comment

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

same here (try to handle only 1 type)

room.game.timer = setTimeout(() => {
room.game.eliminate(room.game.currentPlayer);
game.maxTime = amount;
if (game.timer) {
Copy link
Member

Choose a reason for hiding this comment

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

Not exactly sure whats happening here as I dont see any code thats setting the timer if it was not set previously, maybe this is one of the things typescript caught (timer was set to a non timer truthy value maybe).

chat-plugins/uno.js Show resolved Hide resolved
@KrisXV
Copy link
Member Author

KrisXV commented Aug 27, 2018

i addressed everything

@KrisXV
Copy link
Member Author

KrisXV commented Aug 31, 2018

bump

@HoeenCoder
Copy link
Member

Sorry for the delay, was not aware I was OK to merge this myself.

@HoeenCoder HoeenCoder merged commit e5dcb1f into smogon:master Sep 1, 2018
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