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 support to region BR #56

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

coelholm
Copy link

Add support to Brazil
Minor fix to example codes in the README.md, it was missing "import logging"

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.2% when pulling c30cb18 on coelholm:master into 899220e on kovacsbalu:master.

@coelholm
Copy link
Author

coelholm commented Sep 29, 2021

Without this change it is not possible to calculate routes in Brazil. Is there any reason not to accept this merge? @kovacsbalu
I have interest on this change because I use it with Home Assistant to calculate distances (iCloud3 tracker). Every version update I have to manually change the container and add my version of this code.

@kovacsbalu
Copy link
Owner

@coelholm what do you think about this solution?
base_coord_from_env

@coelholm
Copy link
Author

coelholm commented Oct 1, 2021

@coelholm what do you think about this solution? base_coord_from_env

It will add flexibility to base coordinate and that is good. I believe it is possible to keep env vars persistent in the container of Home Assistant - Device Tracker iCloud3, but I will need to update that every time there is new release of home assistant core.

What do you think about adding it after region as an option? Something like: region,lat,long,coord_server,route_server
That way you can overwrite hard coded configuration and allow for customization.
In my case, instead of configuring region as 'br' I would use 'br,-22.951,-43.210,row-SearchServer/mozi,row-RoutingManager/routingRequest' This way it is possible to keep that customization in a configuration file and it will survive home assistant updates.

Of course It would require some changes of validation code from iCloud3, but I believe it is a minor update and I can propose that as well.

@kovacsbalu
Copy link
Owner

kovacsbalu commented Oct 19, 2021

Hi @coelholm, I see your point but I really don't like too many parameters. Now it has 8 which is a lot. ( I will refact a bit and remove these from init.)
What do you think about the python @property decorator?

@coelholm
Copy link
Author

coelholm commented Nov 4, 2021

Hi @coelholm, I see your point but I really don't like too many parameters. Now it has 8 which is a lot. ( I will refact a bit and remove these from init.) What do you think about the python @property decorator?

Sorry for the delay. So your plan is to remove hard coded configuration related to servers and allow developers to define that on the fly, using @Property methods 'getter, setter, deleter', isn't it? Sound good to me, that way it will be easy to support any area in the world.

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