Skip to content
This repository was archived by the owner on Jun 8, 2024. It is now read-only.

Conversation

danpat
Copy link
Member

@danpat danpat commented Apr 25, 2016

Addresses #183

@TheMarex
Copy link
Member

👍 looks good to me.

@daniel-j-h
Copy link
Member

Why do we only throw an error for the tile parameters here?

Every other service (and the tile function itself) simply asserts on isValid, e.g. see:

https://github.com/Project-OSRM/node-osrm/blob/fix/183/src/node_osrm.cpp#L938

Can we make the behavior consistent? That is removing the assertion (which currently will crash Node in debug builds or quietly work / crash libosrm in release builds --- neither is an option) and throwing Javascript errors instead?

@TheMarex
Copy link
Member

I think this is fine as we usually don't use IsValid because it doesn't give us diagnostics of what made it invalid. In this case there is only one source of errors once you established its integers.

@TheMarex TheMarex merged commit b4a080a into master May 20, 2016
@TheMarex TheMarex deleted the fix/183 branch May 20, 2016 17:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants