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

ability to find countries based on the calling codes #39

Closed
wants to merge 14 commits into from

Conversation

YouriT
Copy link
Collaborator

@YouriT YouriT commented May 3, 2016

I needed that and therefore would love to see it integrated if possible. If it is I will add the according doc!

@therebelrobot
Copy link
Owner

Hey @YouriT! I'm actually gonna be seeing about releasing v2 today, which will change a large portion of the codebase. I like this idea though, and will take a look at implementing it in the next version, I just don't know how mergable this will be after that lands.

@YouriT
Copy link
Collaborator Author

YouriT commented May 3, 2016

Thanks :) well to make it easy, I will take a look at your new code base to integrate this PR or a likewise.

I don't know if you thought about some indexing but it could be nice for searching through the dataset? Personally, I'm not a big fan of O(n) loops when trying to access something quickly. I might be able to deliver it too but I prefer doing it if you could consider merging that feature too?

@therebelrobot
Copy link
Owner

therebelrobot commented May 3, 2016

Take a look at the code in pre-release/v2.0.0 first, and let me know what you think. There are some additional tests needed to be built for it, but the one's I have at the moment are passing. I'm just getting sick of it being "in progress" for over a year :P

@therebelrobot
Copy link
Owner

PR here: #40

Still need to fix linting and see what's failing in the tests.

@YouriT
Copy link
Collaborator Author

YouriT commented May 3, 2016

Yeah linting just burned my eyes!

I made the changes, trying to write the tests but I never used tape, checking that quickly out

@YouriT
Copy link
Collaborator Author

YouriT commented May 3, 2016

Okay, I merged v2 with my PR. Tests passed on my side properly. Tell me if I missed something

@YouriT
Copy link
Collaborator Author

YouriT commented May 3, 2016

Btw, I saw that npm test still resolves to _mocha but that should be changed I guess?

@therebelrobot
Copy link
Owner

I saw that npm test still resolves to mocha but that should be changed I guess?

Yeah, it'll be changed to tape. I've neglected this lib for far too long. Tonight I'll be busting out all what's left/needed for this.

@YouriT
Copy link
Collaborator Author

YouriT commented May 5, 2016

I'm adding some tests, nevertheless, there is a massive breaking change with the responses of the queries. You think that's ok to push it with this new behaviour? I mean, is it mandatory to break that?

@therebelrobot
Copy link
Owner

@YouriT That's why the version will be a major version bump per SemVer. This new behavior allows for searches to return multiple sets of results based if, for example, searching by GB returns 3 results (see #26)

That being said, this discussion should probably be done over on the PR for v2 (#40), unless there's something specific about calling code searches that's needed.

@moimikey moimikey closed this Aug 18, 2021
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