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 a base option to integer filters #387

Merged
merged 1 commit into from
Oct 19, 2016
Merged

Add a base option to integer filters #387

merged 1 commit into from
Oct 19, 2016

Conversation

tfausak
Copy link
Collaborator

@tfausak tfausak commented Oct 15, 2016

This fixes #386.

@tfausak tfausak added the bug label Oct 15, 2016
@tfausak
Copy link
Collaborator Author

tfausak commented Oct 15, 2016

@AaronLasseigne can you review this?

@ngan
Copy link
Contributor

ngan commented Oct 15, 2016

Thanks for the PR! Just a thought: I would imagine everyone out there would now have to add base 10 to their interactions...since it's 99% of the use case out there. I believe this is the same reason JavaScript made base 10 the default for parseInt.

@ngan
Copy link
Contributor

ngan commented Oct 15, 2016

Forgot my point: Should we default it to base 10?

@tfausak
Copy link
Collaborator Author

tfausak commented Oct 15, 2016

I would like to make base: 10 the default, but that'd be a backwards-incompatible change.

@ngan
Copy link
Contributor

ngan commented Oct 15, 2016

I see, makes sense. Can we be sure to make the default 10 on the next "backwards-compability breaking" release?

@AaronLasseigne
Copy link
Owner

Code looks great. We need to add some docs on it to the README.

ngan added a commit to ngan/active_interaction that referenced this pull request Oct 19, 2016
@ngan
Copy link
Contributor

ngan commented Oct 19, 2016

We need to add some docs on it to the README.

Add README update in #391. Thanks! Let's :shipit:

@tfausak tfausak merged commit 1ec511e into master Oct 19, 2016
@tfausak tfausak deleted the gh-386-integer-base branch October 19, 2016 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integer filter handles strings unexpectedly
3 participants