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

Only append query if there is something to append #34

Merged
merged 1 commit into from
Apr 4, 2016

Conversation

irae
Copy link
Contributor

@irae irae commented Apr 3, 2016

This is mostly cosmetic, but it is really strange to have a question
mark if you don't have query, and adding checks on any application
using routr seems wasteful.

i.e. in my application I use: Object.assing({}, detaultQuery, myFuncQueryParam) and sometimes I get an empty object. It would be convenient if the router didn't add the question mark.

r.makePath('my', {wat:'route'}, {});

Before this patch: /my/route?
After this patch: /my/route

This is mostly cosmetic, but it is really strange to have a question
mark if you don't have query, and adding checks on any application
using routr to not pass a query seems wasteful.

i.e. in my application I use:

`Object.assing({}, detaultQuery, myFuncQueryParam)` and sometimes I
get an empty object. It would be convenient if the router didn't add
the question mark.

    r.makePath('my', {wat:'route'}, {});

Before this patch: /my/route?
After this patch: /my/route
@mridgway mridgway added the ready label Apr 3, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c3cd82e on irae:empty-query into 134132e on yahoo:master.

@irae
Copy link
Contributor Author

irae commented Apr 3, 2016

Hello CLA bot? You forgot my PR

@mridgway
Copy link
Collaborator

mridgway commented Apr 4, 2016

CLA bot doesn't work weekends.

👍

@mridgway mridgway merged commit de8796a into yahoo:master Apr 4, 2016
@mridgway mridgway removed the ready label Apr 4, 2016
@irae irae deleted the empty-query branch April 4, 2016 21:31
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.

3 participants