Added query encoding and decoding function #134

Merged
merged 5 commits into from Mar 10, 2014

Conversation

Projects
None yet
2 participants
@jonahbron
Contributor

jonahbron commented Mar 7, 2014

This library contains two simple functions for encoding and decoding query strings.

@joshuacc

This comment has been minimized.

Show comment
Hide comment
@joshuacc

joshuacc Mar 8, 2014

Contributor

I like these methods a lot. However, I think the names might need to be tweaked a bit to avoid confusion with simple query string encoding and decoding.

Contributor

joshuacc commented Mar 8, 2014

I like these methods a lot. However, I think the names might need to be tweaked a bit to avoid confusion with simple query string encoding and decoding.

@jonahbron

This comment has been minimized.

Show comment
Hide comment
@jonahbron

jonahbron Mar 10, 2014

Contributor

Sure, what do you think they should be called?

Contributor

jonahbron commented Mar 10, 2014

Sure, what do you think they should be called?

@joshuacc

This comment has been minimized.

Show comment
Hide comment
@joshuacc

joshuacc Mar 10, 2014

Contributor

Upon reflection, I don't think confusion is all that likely. I was misremembering the names of the built-in browser functions. 😌

Merging as-is. Thanks!

Contributor

joshuacc commented Mar 10, 2014

Upon reflection, I don't think confusion is all that likely. I was misremembering the names of the built-in browser functions. 😌

Merging as-is. Thanks!

joshuacc added a commit that referenced this pull request Mar 10, 2014

Merge pull request #134 from jonahbron/master
Added query encoding and decoding function

@joshuacc joshuacc merged commit 2181eb4 into documentcloud:master Mar 10, 2014

@jonahbron

This comment has been minimized.

Show comment
Hide comment
@jonahbron

jonahbron Mar 10, 2014

Contributor

@joshuacc to me, this seems like a pretty core need in a web application. What would you think about submitting it for inclusion in the core Underscore library?

Also, do you think support for nested data structures should be added, or that it should be kept as is now? For example, giving it this:

// foo[bar][]=example
foo%5Bbar%5D%5B%5D=example

Would be parsed to become:

{
    'foo': {
        'bar': [
            'example'
        ]
    }
}

And vice versa. Right now, if you were to pass a nested data structure to toQuery, the results would be unexpected.

Contributor

jonahbron commented Mar 10, 2014

@joshuacc to me, this seems like a pretty core need in a web application. What would you think about submitting it for inclusion in the core Underscore library?

Also, do you think support for nested data structures should be added, or that it should be kept as is now? For example, giving it this:

// foo[bar][]=example
foo%5Bbar%5D%5B%5D=example

Would be parsed to become:

{
    'foo': {
        'bar': [
            'example'
        ]
    }
}

And vice versa. Right now, if you were to pass a nested data structure to toQuery, the results would be unexpected.

@joshuacc

This comment has been minimized.

Show comment
Hide comment
@joshuacc

joshuacc Mar 10, 2014

Contributor

@jonahbron I agree that this is probably a fairly common need, but Underscore core has a pretty high threshold for determining if a utility should be included. Feel free to suggest it, but I think that having those methods prove themselves in contrib first will be a more successful approach.

Regarding nested data structures: I like the concept, but I'm not familiar with how much support there is for this notation in existing frameworks and libraries. Is there good support for it? Can you point to some sources to confirm that?

Contributor

joshuacc commented Mar 10, 2014

@jonahbron I agree that this is probably a fairly common need, but Underscore core has a pretty high threshold for determining if a utility should be included. Feel free to suggest it, but I think that having those methods prove themselves in contrib first will be a more successful approach.

Regarding nested data structures: I like the concept, but I'm not familiar with how much support there is for this notation in existing frameworks and libraries. Is there good support for it? Can you point to some sources to confirm that?

@jonahbron

This comment has been minimized.

Show comment
Hide comment
@jonahbron

jonahbron Mar 10, 2014

Contributor

@joshuacc yeah, I suppose I'll just let it sit. One of the reasons I like Underscore is because it's so small.

As for that parameter structure, support can be seen for this in jQuery.param(), and every server-side language/framework that I'm aware of handles them that way (PHP, Rails, Django, etc.). I actually wasn't aware of jQuery.param before making this, do you think we should remove the toQuery from this pull-request? If I did that, it would make even more sense to add nested parsing support to fromQuery.

Contributor

jonahbron commented Mar 10, 2014

@joshuacc yeah, I suppose I'll just let it sit. One of the reasons I like Underscore is because it's so small.

As for that parameter structure, support can be seen for this in jQuery.param(), and every server-side language/framework that I'm aware of handles them that way (PHP, Rails, Django, etc.). I actually wasn't aware of jQuery.param before making this, do you think we should remove the toQuery from this pull-request? If I did that, it would make even more sense to add nested parsing support to fromQuery.

@joshuacc

This comment has been minimized.

Show comment
Hide comment
@joshuacc

joshuacc Mar 11, 2014

Contributor

@jonahbron I think that keeping both _.toQuery and _.fromQuery makes sense. We should just add better support for nested data structures. If you have time, go ahead put together another pull request for that.

Thanks!

Contributor

joshuacc commented Mar 11, 2014

@jonahbron I think that keeping both _.toQuery and _.fromQuery makes sense. We should just add better support for nested data structures. If you have time, go ahead put together another pull request for that.

Thanks!

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