search

knadh / dns.toys

A DNS server that offers useful utilities and services over the DNS protocol. Weather, world time, unit conversion etc.
https://www.dns.toys
MIT License
2.48k stars 135 forks source link

Add functionality for calculating Aerial Distances #47

Closed i-sanyam closed 1 year ago

i-sanyam commented 1 year ago

I frequently find myself finding distance between two points on a random website. Will love if this is handled by dns.toys

TODO:

  1. I have yet to add tests.
  2. Really need the delimiter to be , because that's how points are available to me eg. 12.670,34.870
  3. Would appreciate if you take out time for a code review

PS. My first Golang PR

Closes #48

knadh commented 1 year ago

Thanks for the PR @i-sanyam. Can you please clean up the debug statements and send the final version?

sanyam-aggarwal-shipsy commented 1 year ago

Thanks for the PR @i-sanyam. Can you please clean up the debug statements and send the final version?

Sure I will but I have mentioned you in the PR as I am having issue with deciding the delimiter. it isn't working either for - or ,.

i-sanyam commented 1 year ago

https://github.com/knadh/dns.toys/pull/47/files#r1015091961

i-sanyam commented 1 year ago

I have allowed , in the initial query parsing and have tested on the local server by passing all the example queries and it is not breaking. Removed debug statements as well

TODOs done

  1. Added test cases.
  2. Lat Lng Pair Delimiter is ,
knadh commented 1 year ago

Thanks. Will review and merge this week.

knadh commented 1 year ago

@i-sanyam. I've made a few minor cosmetic changes to the commit. Since you send the PR from your master branch, I was unable to amend it, so I've pushed it to a new branch here: https://github.com/knadh/dns.toys/blob/aerial/internal/services/aerial/aerial.go

Please take a look at the changes and if they look fine, I'll merge them.

i-sanyam commented 1 year ago

Thanks @knadh for the edits! I have gone through them. They look fine to me. And before merging them, due to the simplification in error handling, the test cases' error strings need an update. I have added them in a new PR to aerial branch itself here: https://github.com/knadh/dns.toys/pull/52/files

TL;DR the simplified error handling does not validate both the locations' coordinates at once but I think that is fine.

eg in the case of lat long pair - L1 [-91.00, 191.00] L2 [91.00, -191.00]

the user first gets an error that L1 is invalid, the user then fixes L1 and attempts again L1 [11.00, 01.00] L2 [91.00, -191.00] this time user again gets an error that L2 is invalid, which may have been avoided by the user if the error is sent at once for both locations.

i-sanyam commented 1 year ago

Merged in https://github.com/knadh/dns.toys/pull/53/