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

Modified value_for to support arbitrary cases for input booleans. #509

Merged
merged 2 commits into from
Nov 14, 2016

Conversation

ben-zen
Copy link
Contributor

@ben-zen ben-zen commented Oct 26, 2015

This fix addresses #508 by adding case-insensitive comparisons for "true" and "false", while maintaining nil-checking.

@konklone
Copy link
Contributor

Nice contribution -- this PR should solve the problem. I've never seen casecmp before, and it returning an integer value is a little surprising. My stylistic preference would be something like:

if value and ["true", "false"].include?(value.downcase)

Also, I suggest adding/modifying unit tests in query_test.rb and search_test.rb (since the change affects both the Mongo-driven and ES-driven endpoints) to make sure this change in behavior is tested (including with strange inputs, like tRue and "" and other things). Though this looks like a simple enough change, a regression to parameter parsing would be bad.

@ben-zen
Copy link
Contributor Author

ben-zen commented Oct 26, 2015

So, I kept the same basic idea as the original code and just added some nil-checking before calling downcase on the object; I didn't add any test code yet, however, since I'm not particularly familiar with how to work with these tests (including running them), and it looks like the test code didn't even handle boolean parsing at any point in the past. Could you walk me through what you'd suggest for a change, and how to work with it?

@konklone
Copy link
Contributor

I'm replicating your branch locally so I can demonstrate -- tip for next time, it's easier if you also make a new topic branch with a unique name on your fork, then I can switch between work on different origins easily from within the same repo.

@konklone
Copy link
Contributor

#511 has a test added for query parsing, that tests the behavior currently in master.

I tried applying your fix locally to that branch (without committing it) and confirmed that it achieved the desired results. My suggestion to the maintainer is to merge #511, and then anyone can rebase #509 against the new master and add a commit updating the test to move "True" to the success array of test results.

@dwillis dwillis merged commit 28c9b88 into propublica:master Nov 14, 2016
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.

3 participants