Fixed up `serializeArray()` and added multiple support #631

twolfson · 2015-01-19T23:43:48Z

Fixes #235

PR #235 has been open for a while and stagnated. This PR revives adding serializeArray() support and establishes a foundation for other form methods like serialize (#69). In this PR:

Fixed up merge conflicts from Implement .serializeArray() #235
Fixed up broken implementation due to time difference
Added support for multiple selected items (a la jQuery) as requested in Implement .serializeArray() #235
Various style changes to be more consistent with the rest of the repo

…rray Conflicts: lib/cheerio.js

coveralls · 2015-01-19T23:46:38Z

Coverage increased (+0.03%) when pulling 817f84e on underdogio:dev/add.serialize.array.sqwished into f864e39 on cheeriojs:master.

fb55 · 2015-01-24T14:37:26Z

@twolfson Thanks for picking this up from @jlep!

Some notes:

Please be more generous with white space.
It's nice to point to sources, but that can be part of the commit message. While a single source as part of a comment is still fine, you've overdone it a bit.
Please add comments instead regarding the function of code. Keep in mind that the code needs to be maintained indefinitely once this pull request is merged, and you might not be around to explain it.
Please remove dead code (instead of commenting it out).

fb55 · 2015-01-24T14:38:10Z

lib/api/serialize.js

+      var $elem = Cheerio(elem);
+      var name = $elem.attr('name');
+      var val = $elem.val();
+      return val == null ?


This would be much more readable in an if / else block.

That was directly from jQuery. I will break it up.

twolfson · 2015-01-24T21:10:44Z

Have you considered adding jscs to enforce whitespace? I have found it very useful on all of my projects (usually no back and forth about stylistic issues).

http://jscs.info/rules.html

I prefer to leave sources inline since the references to the original code can fade away as more touches occur and git blame erodes. However, I will respect your request.

Most of this code is copy/paste from jQuery but I will do my best to document it.

As for the inline dead code, that was also inline from jQuery but I will make a comment note instead.

jugglinmike · 2015-01-24T21:21:19Z

lib/cheerio.js

@@ -13,7 +13,8 @@ var api = [
  require('./api/attributes'),
  require('./api/traversing'),
  require('./api/manipulation'),
-  require('./api/css')
+  require('./api/css'),
+  require('./api/serialize')


Could you rename the new source file to "forms.js"? This matches jQuery's documented category scheme and will make a logical home for the serializeArray function in the future.

Sure thing. This was originally named api.serialize.js by @jlep so I tried to keep it as consistent as possible.

coveralls · 2015-01-24T21:59:08Z

Coverage increased (+0.04%) to 98.23% when pulling a007441 on underdogio:dev/add.serialize.array.sqwished into e83b10b on cheeriojs:master.

twolfson · 2015-01-24T21:59:12Z

Alright, I have renamed the file so unfortunately most of the inline comments have gone away. I have addressed all of them.

twolfson · 2015-01-24T22:01:47Z

I have re-pushed the branch because Travis CI choked on [email protected] for some reason.

twolfson · 2015-01-24T22:03:41Z

Ah, never mind it isn't me. It looks like master is having a build error.

fb55 · 2015-02-02T23:46:50Z

At this point I would be okay with merging this.

The code would be much simpler when using the :matches selector, but that's something I'll need to implement.

twolfson · 2015-02-02T23:56:49Z

Sweet, I see that master has been patched. I am going to wiggle my PR build.

coveralls · 2015-02-03T00:03:26Z

Coverage increased (+0.04%) to 98.23% when pulling 30bce68 on underdogio:dev/add.serialize.array.sqwished into 64fe78e on cheeriojs:master.

twolfson · 2015-02-03T00:03:47Z

Alright, the PR has been wiggled and Travis CI is now green.

jugglinmike · 2015-03-01T03:01:23Z

I think this is almost there; it's just missing one piece of functionality that I can see. From the jQuery docs:

This method can act on a jQuery object that has selected individual form controls, such as <input>, <textarea>, and <select>.

That means before we land this, the patch should be updated to pass a test like:

+       it('() :', function() {
+         expect($('#nested input').serializeArray()).to.eql([
+            { name: 'fruit', value: 'Apple' },
+            { name: 'vegetable', value: 'Carrot' }
+          ]);
+       });

@twolfson It's been a while since anybody gave you feedback on this. If you'd moved on to bigger and better things, just let me know and I'll be happy to finish this up for ya

twolfson · 2015-03-01T03:05:14Z

Sure thing. I have a bit of time to kill before heading out tonight. I will take a shot at those new tests and comment when this PR is updated.

jugglinmike · 2015-03-01T03:13:18Z

That's some stick-to-itiveness for you! Thanks :)

twolfson · 2015-03-01T03:39:32Z

Alright, I have added a new test and fix. I also saw that the tests are very unclear about what results should be. I have refactored them to be much more about asserting deep equals of arrays/objects to make the return value more obvious.

coveralls · 2015-03-01T03:41:04Z

Coverage increased (+0.04%) to 98.61% when pulling 7a03e15 on underdogio:dev/add.serialize.array.sqwished into 55f7ac0 on cheeriojs:master.

jugglinmike · 2015-03-01T03:57:21Z

lib/api/forms.js

+        return $elem.filter(submittableSelector).toArray();
+      }
+    })
+    .filter(function() {


Could you move this method call to the previous line for visual consistency in the chain?

Whoops, sure thing

coveralls · 2015-03-01T04:00:36Z

Coverage increased (+0.04%) to 98.61% when pulling 08f2771 on underdogio:dev/add.serialize.array.sqwished into 55f7ac0 on cheeriojs:master.

jugglinmike · 2015-03-01T04:55:27Z

Rebased and merged to master via gh-669. Thanks for sticking with us, Todd!

twolfson · 2015-03-01T22:31:02Z

Woot, no problem. Glad to see it got landed. There was also the follow-up PR #632 which I will update in a bit to have a much cleaner git history.

Jarno Leppänen and others added 8 commits July 5, 2013 14:17

Implement serializeArray()

81ff6a2

Import serialization api

92a7161

Added forms test fixture

b3dfcc9

Added test cases for .serializeArray()

91d1c86

Added newlines at the end file where missing

fe9b031

fixed .serializeArray for tags with uppercase names

a6e265f

Merge remote-tracking branch 'origin/master' into dev/add.serialize.a…

405d6a4

…rray Conflicts: lib/cheerio.js

Repairing overly aggressive merge

e3fd94a

twolfson mentioned this pull request Jan 19, 2015

Implement .serializeArray() #235

Closed

twolfson mentioned this pull request Jan 20, 2015

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

Closed

fb55 reviewed Jan 24, 2015
View reviewed changes

fb55 mentioned this pull request Jan 24, 2015

Support :matches selector fb55/css-select#28

Closed

jugglinmike reviewed Jan 24, 2015
View reviewed changes

twolfson force-pushed the dev/add.serialize.array.sqwished branch from 817f84e to a007441 Compare January 24, 2015 21:57

twolfson force-pushed the dev/add.serialize.array.sqwished branch from a007441 to 196b2d0 Compare January 24, 2015 22:01

twolfson force-pushed the dev/add.serialize.array.sqwished branch 3 times, most recently from 96177ef to 30bce68 Compare February 3, 2015 00:02

fb55 mentioned this pull request Feb 27, 2015

Change lodash dependencies to ^3.2.0 #655

Merged

twolfson force-pushed the dev/add.serialize.array.sqwished branch from 30bce68 to 7a03e15 Compare March 1, 2015 03:38

jugglinmike reviewed Mar 1, 2015
View reviewed changes

Fixed up serializeArray() and added multiple support

08f2771

twolfson force-pushed the dev/add.serialize.array.sqwished branch from 7a03e15 to 08f2771 Compare March 1, 2015 03:59

jugglinmike mentioned this pull request Mar 1, 2015

Serialize array #669

Merged

jugglinmike closed this Mar 1, 2015

fb55 mentioned this pull request Mar 1, 2015

Use a selector to filter form elements #670

Closed

twolfson deleted the dev/add.serialize.array.sqwished branch March 1, 2015 22:42

twolfson mentioned this pull request Mar 11, 2016

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed up `serializeArray()` and added multiple support #631

Fixed up `serializeArray()` and added multiple support #631

twolfson commented Jan 19, 2015

coveralls commented Jan 19, 2015

fb55 commented Jan 24, 2015

fb55 Jan 24, 2015

twolfson Jan 24, 2015

twolfson commented Jan 24, 2015

jugglinmike Jan 24, 2015

twolfson Jan 24, 2015

coveralls commented Jan 24, 2015

twolfson commented Jan 24, 2015

twolfson commented Jan 24, 2015

twolfson commented Jan 24, 2015

fb55 commented Feb 2, 2015

twolfson commented Feb 2, 2015

coveralls commented Feb 3, 2015

twolfson commented Feb 3, 2015

jugglinmike commented Mar 1, 2015

twolfson commented Mar 1, 2015

jugglinmike commented Mar 1, 2015

twolfson commented Mar 1, 2015

coveralls commented Mar 1, 2015

jugglinmike Mar 1, 2015

twolfson Mar 1, 2015

twolfson Mar 1, 2015

coveralls commented Mar 1, 2015

jugglinmike commented Mar 1, 2015

twolfson commented Mar 1, 2015

Fixed up serializeArray() and added multiple support #631

Fixed up serializeArray() and added multiple support #631

Conversation

twolfson commented Jan 19, 2015

coveralls commented Jan 19, 2015

fb55 commented Jan 24, 2015

fb55 Jan 24, 2015

Choose a reason for hiding this comment

twolfson Jan 24, 2015

Choose a reason for hiding this comment

twolfson commented Jan 24, 2015

jugglinmike Jan 24, 2015

Choose a reason for hiding this comment

twolfson Jan 24, 2015

Choose a reason for hiding this comment

coveralls commented Jan 24, 2015

twolfson commented Jan 24, 2015

twolfson commented Jan 24, 2015

twolfson commented Jan 24, 2015

fb55 commented Feb 2, 2015

twolfson commented Feb 2, 2015

coveralls commented Feb 3, 2015

twolfson commented Feb 3, 2015

jugglinmike commented Mar 1, 2015

twolfson commented Mar 1, 2015

jugglinmike commented Mar 1, 2015

twolfson commented Mar 1, 2015

coveralls commented Mar 1, 2015

jugglinmike Mar 1, 2015

Choose a reason for hiding this comment

twolfson Mar 1, 2015

Choose a reason for hiding this comment

twolfson Mar 1, 2015

Choose a reason for hiding this comment

coveralls commented Mar 1, 2015

jugglinmike commented Mar 1, 2015

twolfson commented Mar 1, 2015

Fixed up `serializeArray()` and added multiple support #631

Fixed up `serializeArray()` and added multiple support #631