norydev / hitchspots

Get Hitchwiki spots into MAPS.me
http://www.hitchspots.me
MIT License
18 stars 2 forks source link

Add possibility to add route intermediate points. Fixes #18 #20

Closed serge1peshcoff closed 5 years ago

serge1peshcoff commented 6 years ago

Node: In the current state it's totally not for merging, this is WIP for now.

@NoryDev to implement adding intermediate points, the rewriting of the internal API was required, so I've did it. What I did for now is changed the internal API for the project, and it's quite big changes, just wanted to check if it's okay by you and get your opinion on that. (Everything that was working before should work for now as well (the browser version is working, the tests and Rubocop reports are also fine), this is basically the base for future improvement.)

I didn't add the new features yet and I will add them a little bit later and write about it here.

serge1peshcoff commented 6 years ago

Also, my idea is to change the API of this library slightly, so the request to /trip would accept the array of points instead of from and to params. My idea is to implement it so the request would look like that:

/trip?places[0][name]=Voronezh,+city,+Russia&places[0][lat]=51.6548797&places[0][lon]=39.212949&places[1][name]=Moscow,+city,+Russia&places[1][lat]=55.7507178places[1][lon]=37.6176606

instead of

trip?from=Voronezh,+city,+Russia&from_lat=51.6548797&from_lon=39.212949&to=Moscow,+city,+Russia&to_lat=55.7507178&to_lon=37.6176606

That way it would be possible to send arbitrary amount of points to Hitchspots instead of just 2. What do you think?

norydev commented 6 years ago

Hi @serge1peshcoff, thanks for the PR, I will have a look at your proposition this weekend, I don't have much time until then.

serge1peshcoff commented 6 years ago

@NoryDev another thing I've just realised is the /trip API can be done that way:

/trip?places[][name]=Krakow&places[][lat]=333&places[][lon]=21&places[][name]=Kyiv&places[][name]=Kharkiv&places[][name]=Voronezh

without specifying the indexes, Sinatra understands it perfectly.

norydev commented 6 years ago

@serge1peshcoff I had a look. That's a good proposition 👍 You can go ahead and implement the interface as well.

I must say that I prefer the first one:

/trip?places[0][name]=Voronezh,+city,+Russia&places[0][lat]=51.6548797&places[0][lon]=39.212949&places[1][name]=Moscow,+city,+Russia&places[1][lat]=55.7507178places[1][lon]=37.6176606

Because the order is important. So I think it's better if the API reflects that.

norydev commented 5 years ago

Hey @serge1peshcoff, since I didn't hear from you and still liked your feature idea, I have done this change in #23

serge1peshcoff commented 5 years ago

@NoryDev that is really lovely, thanks! You've wrote that there's no visual changes, I'll try to make another PR on that. (Sorry for not answering, had really tough time busy with other important stuff and forgot about this one)

norydev commented 5 years ago

@serge1peshcoff no prob 👍