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 country search by numeric code #137

Merged
merged 2 commits into from
Aug 18, 2016
Merged

Conversation

Envek
Copy link
Contributor

@Envek Envek commented Mar 1, 2014

This pull request extends #125 allowing not only access country's numeric code value but also allows to search countries by numeric code.

@Envek
Copy link
Contributor Author

Envek commented Mar 1, 2014

There is one difficulty: user may pass numeric code as an integer to Carmen::Country.coded. Currently Carmen will throw an exception (trying to downcase a number), but if we allow user to pass numbers, there will be another difficulty - user may pass number with leading zero and Ruby will assume that there is an octal number and search results will be incorrect.

I've wrote handling code, that will accept numbers, but also will issue a warning, discouraging passing numbers. You can see it here: https://github.com/Envek/carmen/compare/numeric_code_search...handle_integer_numeric_code_searches?expand=1

I'm dislikes it in common, so, not sure, whether include it in this pull request or not.

@jim
Copy link
Collaborator

jim commented Mar 3, 2014

I think it's probably best to have multiple methods that search by a specific field instead of one that tries to handle all of the cases.

Possibly, we should rename coded to alpha_coded and add a new numerically_coded (or numeric_coded) method to search by numeric string. We would need to preserve the current API by aliasing coded to alpha_coded for the near future.

As far as handling a user passing an octal number accidentally, I think it's fine to not handle that case. Sometimes it is nice to help users do the right thing, but I think it's OK to expect them to understand the various numeric syntaxes in Ruby.

@Envek
Copy link
Contributor Author

Envek commented Mar 24, 2014

So, I've added multiple methods that search by a specific field, but keep Country::coded to search by all (including numeric code). Also, I've written detailed description in README, with warning about numeric syntaxes in ruby.

@matsubo
Copy link
Contributor

matsubo commented Apr 8, 2014

+1

@jim
Copy link
Collaborator

jim commented Oct 26, 2014

This looks good. I will merge this before I release the next version.

@ecbypi ecbypi mentioned this pull request Mar 17, 2015
@ecbypi
Copy link
Contributor

ecbypi commented Mar 17, 2015

@cdainmiller Thoughts on this PR? I had some thoughts on this outlined in #170.

@ghost
Copy link

ghost commented Mar 18, 2015

Thanks @ecbypi - I'll follow up in #170

@ghost
Copy link

ghost commented Apr 23, 2015

@jim What should we do here? Still thinking this looks good for a release? I'm not sure, but maybe this is something we want to merge into a version-2.0 branch instead of master?

@Envek
Copy link
Contributor Author

Envek commented Mar 13, 2016

Wow, this PR is already two years old and still mergeable! @jim, what do you think about it now?

@jim
Copy link
Collaborator

jim commented Aug 18, 2016

I think I intended to merge this a while ago. Sorry for the delay, @Envek. It's a small change to the existing API, and as several uses will find this useful, that's a good enough reason for me.

@jim jim merged commit cd3e696 into carmen-ruby:master Aug 18, 2016
@Envek Envek deleted the numeric_code_search branch August 19, 2016 04:28
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.

4 participants