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

Move most of the parameters for queryTrips in new TripOptions class #214

Closed
ialokim opened this issue Aug 1, 2018 · 6 comments
Closed

Comments

@ialokim
Copy link
Contributor

ialokim commented Aug 1, 2018

This is just a follow up on #102 (comment) to discuss the idea.

I've had a quick look which of the parameters could be moved into a new TripOptions class. I would leave from, to and date as normal parameters, while moving all the others into the new class (dep, products, optimize, walkSpeed, accessibility, options) because they seem quite "auxiliary". Would you be ok with this distribution @schildbach ?

@schildbach
Copy link
Owner

Sounds good to me, except dep. It's kind of tied to date.

@ialokim
Copy link
Contributor Author

ialokim commented Aug 1, 2018

Okay, so we would leave dep as normal parameter too (and would have via as auxiliary option, which I forgot to mention above)?

@schildbach
Copy link
Owner

I'm a bit indifferent to via, but yes. What do others think?

@grote
Copy link
Contributor

grote commented Aug 1, 2018

While I think this change would be an improvement, I don't think it is worth to break the API without a strong argument. Making it prettier isn't a strong argument that would justify the re-factoring required for the consumers of the library. Maybe the old API can continue to exist and get deprecated, but converted to the new format?

I'm a bit indifferent to via, but yes. What do others think?

I'd leave via out of the options and instead leave it between from and to.

@schildbach
Copy link
Owner

I'm fine with keeping a deprecated old version of the method.

@ialokim
Copy link
Contributor Author

ialokim commented Sep 13, 2020

Fixed with #216.

@ialokim ialokim closed this as completed Sep 13, 2020
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

No branches or pull requests

3 participants