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

decodeQuery does not work as described in docs.html #137

Closed
hankhero opened this Issue Mar 5, 2014 · 1 comment

Comments

Projects
None yet
2 participants
@hankhero

hankhero commented Mar 5, 2014

If you run the examples in the docs, docs.html#static-decodeQuery, they don't work as explained. Here are two, currently failing, tests I made based on docs.html that shows it. Also, the functions in this section are lacking unit-tests. I volunteer to help code and fix, but then I need some help explain how the functions are specified to work. Is the docs or the code faulty?

test("encodeQuery", function () {
    equal(URI.encodeQuery(" "), "+");
    equal(URI.encode(" "), "%20");
});

test("decodeQuery", function () {
    equal(URI.decodeQuery("+"), " ");
    equal(URI.decode("+"), "+");
});
@rodneyrehm

This comment has been minimized.

Show comment
Hide comment
@rodneyrehm

rodneyrehm Mar 5, 2014

Member

This is an issue with the documentation and the result of a bad API decision.

Version 1.11.0 introduced URI.escapeQuerySpace so one could configure if a querystring should use + or %20 for encoding spaces.

in Order to get the the result you're looking for, you'd have to call URI.encodeQuery(" ", true). From an API point of view this doesn't make any sense. The additional parameter was meant for internal use. It should be optional when used externally. In order to make this work, I would change

URI.encodeQuery = function(string, escapeQuerySpace) {
    var escaped = URI.encode(string + "");
    return escapeQuerySpace ? escaped.replace(/%20/g, '+') : escaped;
};

to

URI.encodeQuery = function(string, escapeQuerySpace) {
    var escaped = URI.encode(string + "");
    if (escapeQuerySpace === undefined) {
        escapeQuerySpace = URI.escapeQuerySpace;
    }
    return escapeQuerySpace ? escaped.replace(/%20/g, '+') : escaped;
};

Would that conform with your expectations?

Member

rodneyrehm commented Mar 5, 2014

This is an issue with the documentation and the result of a bad API decision.

Version 1.11.0 introduced URI.escapeQuerySpace so one could configure if a querystring should use + or %20 for encoding spaces.

in Order to get the the result you're looking for, you'd have to call URI.encodeQuery(" ", true). From an API point of view this doesn't make any sense. The additional parameter was meant for internal use. It should be optional when used externally. In order to make this work, I would change

URI.encodeQuery = function(string, escapeQuerySpace) {
    var escaped = URI.encode(string + "");
    return escapeQuerySpace ? escaped.replace(/%20/g, '+') : escaped;
};

to

URI.encodeQuery = function(string, escapeQuerySpace) {
    var escaped = URI.encode(string + "");
    if (escapeQuerySpace === undefined) {
        escapeQuerySpace = URI.escapeQuerySpace;
    }
    return escapeQuerySpace ? escaped.replace(/%20/g, '+') : escaped;
};

Would that conform with your expectations?

@rodneyrehm rodneyrehm added the Bug label Mar 5, 2014

@rodneyrehm rodneyrehm closed this in be0bfc8 Mar 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment