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

Added .serialize() support. Fixes #69 #632

Conversation

twolfson
Copy link
Contributor

Now that #631 is implemented, we can add .serialize() support. Unfortunately, this comes with adding $.param(). $.param has a second half that .serialize() doesn't leverage (see its else statement). I am not sure whether we want to add this/test it, or drop it and make $.param internal only for now.

In this PR:

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.39%) when pulling cc71303 on underdogio:dev/add.serialize.method.sqwished into f864e39 on cheeriojs:master.

@twolfson
Copy link
Contributor Author

twolfson commented Mar 1, 2015

I have caught up this PR with the changes from landing #631 to master. It looks like I was wreckless when adding tests (i.e. didn't test all of jQuery's functionality) and was focused on using serialize to get a query string. I will revisit this later.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.42%) to 97.2% when pulling c855375 on underdogio:dev/add.serialize.method.sqwished into ccd9369 on cheeriojs:master.

@twolfson twolfson force-pushed the dev/add.serialize.method.sqwished branch from c855375 to ddd4fd0 Compare March 2, 2015 03:31
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.62% when pulling ddd4fd0 on underdogio:dev/add.serialize.method.sqwished into ccd9369 on cheeriojs:master.

@twolfson
Copy link
Contributor Author

twolfson commented Mar 2, 2015

I have chosen to take the shorter path and stripped out everything from jQuery.param that is irrelevant to .serialize(). The result is a much smaller PR. There is 1 line I haven't been able to figure out how to test which is:

var value = data.value == null ? '' : data.value;

I cannot seem to find a case when data.value == null. .serializeArray() returns no data for val == null.

return null;

@fb55
Copy link
Member

fb55 commented Mar 10, 2015

@twolfson I can't think of a case where data.value is null either, just drop the line. The rest LGTM.

@twolfson twolfson force-pushed the dev/add.serialize.method.sqwished branch from ddd4fd0 to dcd6680 Compare March 11, 2015 04:33
@twolfson
Copy link
Contributor Author

I have updated the PR to ditch the null check.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.62% when pulling dcd6680 on underdogio:dev/add.serialize.method.sqwished into f71387e on cheeriojs:master.

@mh166
Copy link

mh166 commented Mar 11, 2016

@twolfson, @fb55: is there any news on this PR? It would be very useful to me in my current project. 👍

@twolfson
Copy link
Contributor Author

There's a merge conflict and I no longer have write access to the source repo. I'm going to close and reopen this PR.

@mh166 You can point to a git URL for dependencies in npm:

https://docs.npmjs.com/files/package.json#urls-as-dependencies

"cheerio": "git://github.com/underdogio/cheerio#dcd66809"

@twolfson
Copy link
Contributor Author

I have opened the replacement PR #827

@mh166
Copy link

mh166 commented Mar 14, 2016

Thanks! It's greatly appreciated!

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

Successfully merging this pull request may close these issues.

4 participants