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

Distance and time calculation #340

Merged
merged 18 commits into from
May 27, 2023
Merged

Distance and time calculation #340

merged 18 commits into from
May 27, 2023

Conversation

nikdav
Copy link
Contributor

@nikdav nikdav commented May 15, 2023

Everything should be explained in the comments but here's a tl;dr:

  • Adds new item attributes for distance + time calculation for walking, driving, biking and public transport to the item/store location (walking_dt, driving_dt, biking_dt, transit_dt)
  • Time and distance is the first "best" route found by Google Maps (not considering traffic)
  • "LOCATION" needs to be enabled in config and Google Maps API key generated & (Home/Origin address) entered by user
  • API requests are very low because of caching and only firing requests when they are needed

Example:
image

Tests weren't written because an API-Key would be needed.
"make executable" doesn't work regardless of my changes (because Windows?):
[...]
cp src/config.sample.ini dist/config.ini
process_begin: CreateProcess(NULL, cp src/config.sample.ini dist/config.ini, ...) failed.
make (e=2): Das System kann die angegebene Datei nicht finden.
make: *** [executable] Fehler 2

Pull Request Checklist:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Did you make your Pull Request on the dev branch?
  • Does your submission pass tests? make test
  • Have you lint your code locally prior to submission? make lint
  • Could you build and run the docker image successfully? make image
  • Could you create a running executable? make executable
  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran manual tests with your changes locally?

@Der-Henning
Copy link
Owner

Hi @nikdav.
Thank you for your contribution.
I will have a deeper look into your code soon and create a review.

Please only change the code related to your feature. Changing the formatting of other parts of the code may cause merge conflicts.

To reduce API requests I suggest you implement the new item attributes (walking_dt, etc.) as properties and only calculate them, including the Google API request when the property is requested by the notifiers.

@Der-Henning
Copy link
Owner

Hi @nikdav.
I tested your code and it works as expected and would like to merge it into dev.
Please merge the latest commits from the dev branch to your feature branch and make sure pre-commit run -a throws no exceptions.

@nikdav
Copy link
Contributor Author

nikdav commented May 26, 2023

Hi @Der-Henning
Forgot to fix the formatting in the config.py - fixed now. I also merged the latest commits from the dev branch and
pre-commit run -a throws no exceptions. Ready for the merge :)

@Der-Henning Der-Henning merged commit a8dcd44 into Der-Henning:dev May 27, 2023
2 of 11 checks passed
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.

None yet

2 participants