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.

Sign up for GitHub

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

Jump to bottom

[10.0][MIG] geoengine_base_geolocalize migration #155

Merged
merged 7 commits into from
Jun 19, 2020
Merged

[10.0][MIG] geoengine_base_geolocalize migration #155

merged 7 commits into from
Jun 19, 2020

Conversation

benwillig
Copy link
Contributor

Also moved the openstreetmap api call in specific module.

@lmignon Your review is welcome!

@benwillig benwillig force-pushed the 10.0-mig_base_geolocalize-bwi branch from fa0bbc0 to 4b0d5c4 Compare October 2, 2017 11:47
JonathanNEMRY
JonathanNEMRY reviewed Oct 3, 2017
View reviewed changes
base_geolocalize_openstreetmap/models/geolocalize_openstreetmap.py Outdated
"""Get the latitude and longitude by requesting "mapquestapi"
see http://open.mapquestapi.com/geocoding/
"""
url = 'http://nominatim.openstreetmap.org/search'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as it is an openstreetmap class then could it make sens to externalize this one into a class attribute _url ?? (in order to reuser request into other call?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. I also thought about using a config parameter but I think it doesn't make sense as this URL will maybe never change.

JonathanNEMRY
JonathanNEMRY approved these changes Oct 3, 2017
View reviewed changes
Copy link

@JonathanNEMRY JonathanNEMRY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a litle question otherwise seems ok

@benwillig benwillig force-pushed the 10.0-mig_base_geolocalize-bwi branch from 4b0d5c4 to ad9bece Compare October 3, 2017 13:35
@lmarion-source lmarion-source force-pushed the 10.0-mig_base_geolocalize-bwi branch 2 times, most recently from 527e215 to 2051fc8 Compare June 2, 2020 13:08
lmignon
lmignon approved these changes Jun 15, 2020
View reviewed changes
Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm in production. Thank you @lmarion-source

@lmignon lmignon force-pushed the 10.0-mig_base_geolocalize-bwi branch from 2051fc8 to 2050be1 Compare June 19, 2020 13:25
@lmarion-source @lmignon
[FIX] code format and pylint:
6eea371 
Squashed commit of the following:

commit 527e215
Author: Lindsay <[email protected]>
Date:   Tue Jun 2 14:47:52 2020 +0200

    fixing typo

commit dd9c3ec
Author: Lindsay <[email protected]>
Date:   Tue Jun 2 14:42:47 2020 +0200

    fixing pylint travis

commit 7899f9b
Author: Lindsay <[email protected]>
Date:   Tue Jun 2 14:05:25 2020 +0200

    fix travis flake8 linting

commit 039e867
Author: Lindsay <[email protected]>
Date:   Tue Jun 2 13:54:07 2020 +0200

    fix travis pylint

commit fcfb09e
Author: Lindsay <[email protected]>
Date:   Tue Jun 2 13:45:09 2020 +0200

    fixing travis tests

commit ff2b39e
Author: Lindsay <[email protected]>
Date:   Tue Jun 2 12:07:45 2020 +0200

    fix import

commit cdf43dc
Author: Lindsay <[email protected]>
Date:   Tue Jun 2 11:47:21 2020 +0200

    add responses to travis for testing

commit 79da0ee
Author: Lindsay <[email protected]>
Date:   Tue Jun 2 11:40:55 2020 +0200

    disable pylint warning W0622

commit d6eab49
Author: Lindsay <[email protected]>
Date:   Tue Jun 2 10:29:58 2020 +0200

    formatting for linters
@lmignon lmignon force-pushed the 10.0-mig_base_geolocalize-bwi branch from 2050be1 to 6eea371 Compare June 19, 2020 13:29
@lmignon
Copy link
Contributor

lmignon commented Jun 19, 2020

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 10.0-ocabot-merge-pr-155-by-lmignon-bump-patch, awaiting test results.

@lmignon lmignon mentioned this pull request Jun 19, 2020
Closed
2 tasks
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@OCA-git-bot OCA-git-bot merged commit 4435ddf into OCA:10.0 Jun 19, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 317017d. Thanks a lot for contributing to OCA. ❤️

@len-foss len-foss deleted the 10.0-mig_base_geolocalize-bwi branch December 1, 2020 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmignon lmignon lmignon approved these changes

@JonathanNEMRY JonathanNEMRY JonathanNEMRY approved these changes

@ThomasBinsfeld ThomasBinsfeld ThomasBinsfeld approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@benwillig @lmignon @OCA-git-bot @JonathanNEMRY @ThomasBinsfeld @asaunier @lmarion-source