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

Only coerce ids that are all digits into integers #1052

Merged
merged 1 commit into from Jan 20, 2016

Conversation

technicalpickles
Copy link
Member

This lets you set a HUBOT_SHELL_USER_ID=U0123456, as if to pretend to
have a Slack user id.

Without this change, parseInt ends up returning undefined for strings like U0123456.

This lets you set a HUBOT_SHELL_USER_ID=U0123456, as if to pretend to
have a Slack user id.
@michaelansel
Copy link
Collaborator

If we go this route, shouldn't we just make the IDs always strings instead of needing to deal with both data types? Probably doesn't matter much, but having mixed data types is always weird...

@technicalpickles
Copy link
Member Author

In practice, IDs always seem to be strings. I inspected our bot with the Campfire adapter, and the keys are strings of integers. Slack is strings already as I said. That leaves shell being the weird one, so I guess they should just always be strings.

@technicalpickles
Copy link
Member Author

The only annoyance with making them alwasys be strings in the shell adapter is that then there are brains out in the wild with integer keys, so that could be a bit of a surprise after an upgrade.

@michaelansel
Copy link
Collaborator

My gut says that is okay as a breaking change since the shell adapter is never(?) used as a production adapter, but I do see the potential breakage...

technicalpickles added a commit that referenced this pull request Jan 20, 2016
Only coerce ids that are all digits into integers
@technicalpickles technicalpickles merged commit c209c3a into master Jan 20, 2016
@technicalpickles technicalpickles deleted the shell-user-id-can-be-non-int branch January 20, 2016 03:46
@technicalpickles technicalpickles mentioned this pull request Jan 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants