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

Implements prefer english names setting for MapQuest API in config.ini #290

Merged
merged 8 commits into from Jan 22, 2019

Conversation

Projects
None yet
4 participants
@DZamataev
Copy link
Contributor

DZamataev commented Dec 26, 2018

Which adds English as accepted header to MapQuest requests and results in the response location names being mostly in English not their native country language.

Just to illustrate the case take a look at my terminal logs and the names of the folders changing from Russian and Arabic language into English after setting the new flag in config.ini using nano editor.

Before

$ nano ~/.elodie/config.ini ### I've set prefer_english_names=False

screen shot 2018-12-26 at 16 31 46

After

$ nano ~/.elodie/config.ini ### I've set prefer_english_names=True
$ rm ~/.elodie/location.json
$ rm ~/.elodie/hash.json

screen shot 2018-12-26 at 16 32 13

Implements prefer english names setting in config.ini which adds Engl…
…ish as accepted header to MapQuest requests and results in the response location names being mostly in English not their native country language.
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Dec 26, 2018

CLA assistant check
All committers have signed the CLA.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 26, 2018

Coverage Status

Coverage decreased (-2.002%) to 91.122% when pulling 4142dab on DZamataev:master into 91bf181 on jmathai:master.

@jmathai jmathai added the enhancement label Dec 26, 2018

@jmathai
Copy link
Owner

jmathai left a comment

Thanks for the PR. Looks great. I left a couple comments.

Also, before merging it would be great to have some tests. One with prefer english names as False and another for the same city with `True. Here's an example test.

Would also be great to have a test for the geolocation.get_prefer_english_name(). (I noticed there are no tests for geolocation.get_key() to use as an example....

Show resolved Hide resolved Readme.md Outdated
Show resolved Hide resolved elodie/geolocation.py Outdated
Show resolved Hide resolved elodie/geolocation.py Outdated
@DZamataev

This comment has been minimized.

Copy link
Contributor

DZamataev commented Dec 27, 2018

Sorry but i'm not much into Python to write good enough unit tests. Will account for the other changes requested and update.

@jmathai

This comment has been minimized.

Copy link
Owner

jmathai commented Dec 27, 2018

Sounds good. Why don't you make the suggested changes and I will submit a PR to your branch with tests and then I can merge it.

@DZamataev DZamataev changed the title Implements prefer english names setting in config.ini Implements prefer english names setting in config.ini and separating media in different folders according to it's type Dec 28, 2018

@DZamataev

This comment has been minimized.

Copy link
Contributor

DZamataev commented Dec 28, 2018

@jmathai I've also added command line argument to put files in different folders like so:
screen shot 2018-12-28 at 14 20 04

@DZamataev

This comment has been minimized.

Copy link
Contributor

DZamataev commented Jan 15, 2019

@jmathai Hello! I'm done with the changes. Please check this out and consider merging!

@DZamataev

This comment has been minimized.

Copy link
Contributor

DZamataev commented Jan 15, 2019

@noonat @zserg take a look please.

@jmathai

This comment has been minimized.

Copy link
Owner

jmathai commented Jan 18, 2019

Thanks! I added tests and submitted a PR back to your fork.

I removed the commits which add support for media folders. Let's create a new PR for that and have it be one of the configurable options when specifying how your directory is structured.

@jmathai jmathai changed the title Implements prefer english names setting in config.ini and separating media in different folders according to it's type Implements prefer english names setting in config.ini Jan 22, 2019

@jmathai jmathai changed the title Implements prefer english names setting in config.ini Implements prefer english names setting for MapQuest API in config.ini Jan 22, 2019

@jmathai jmathai merged commit e5af0df into jmathai:master Jan 22, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-2.002%) to 91.122%
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment