Removing xml2json dependency for xml2js for smaller dependency footprint...

[cromestant]

xml2json module builds lots of dependencie sand is huge. Unless I missed something ( tests pass) it is not required and can do with this smaller footprint xml2js module.

[markstos]

@robludwig Do you agree that it's safe to replace xml2json with xml2js here?

[cromestant]

just to be clear the xml2json project is great, but in this case is not really required. Do run other tests if you have others than themake test , but everything is running for me.

[robludwig]

fair enough. can we peg the version of xml2js to something more specific?

[cromestant]

Refined to current minor.

[robludwig]

:+1:

[cromestant]

Hello, any updates on merging this PR?

[markstos]

@cromestant It appears there is not test coverage for returning error messages as JavaScript objects. Please add this test coverage to the PR to confirm the package change doesn't change what's returned.

[cromestant]

added better parsing and fixed a test.

hope I did not misunderstand your request.

[markstos]

Thanks, I'll take a look.

On Mon, Apr 13, 2015 at 6:05 PM, cromestant notifications@github.com wrote:

added better parsing and fixed a test.

hope I did not misunderstand your request.

Reply to this email directly or view it on GitHub https://github.com/aheckmann/node-ses/pull/15#issuecomment-92513886.

Mark Stosberg
Senior Systems Engineer
RideAmigos

[markstos]

I found the patch had a bug in it. It lacked some return calls, so that when the XML error parsing was invoked, the callback would be called twice, once as an "error" and then a second time as "success".

I added new test coverage to check for this and also fixed the issue.

I squashed the various commits and merged them into master:

https://github.com/aheckmann/node-ses/commit/600e9013d630f04fa08ea83b09017be7322298c3

Unless @aheckmann or @robludwig object in the next day or so, I'll publish the new version which replaces the xml2json dependency with xml2js.

The new version I prepared also bumps our other dependencies as long as we are doing some maintenance.

[cromestant]

great, question on githug etiquette, should I close this pull request now? or wait until merge? Also now it states that there are conflicts, should I just rebase on yours?

[markstos]

I'll close the pull request when I release soon, as there's still an open comment period if Rob or Aaron have any final comments.

Regarding the commits, I recommend using your own branch until the release happens, and then switching to using the released version. At that point, you can delete your branch.

[aheckmann]

LGTM

[markstos]

Releasing today, so closing.