-
Notifications
You must be signed in to change notification settings - Fork 37
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
Removing xml2json dependency for xml2js for smaller dependency footprint... #15
Conversation
@robludwig Do you agree that it's safe to replace xml2json with xml2js here? |
just to be clear the xml2json project is great, but in this case is not really required. |
fair enough. can we peg the version of xml2js to something more specific? |
Refined to current minor. |
👍 |
Hello, any updates on merging this PR? |
@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. |
…ponse objects to not use arrays and to not wrap attrs in an object, so as to mimic previous lib's behavior ( although it was not needed). * fixed final test where message was misspeled ( capitalization). * Fixed final assert message as I did not find an instance where that string could be returned ('node-ses failed with status: 403 and data') fixed it with the default string, although test should be improved as with config this will not be the response. current build throws no test errors on this end.
added better parsing and fixed a test. hope I did not misunderstand your request. |
Thanks, I'll take a look. On Mon, Apr 13, 2015 at 6:05 PM, cromestant notifications@github.com
|
I found the patch had a bug in it. It lacked some I added new test coverage to check for this and also fixed the issue. I squashed the various commits and merged them into master: 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. |
great, |
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. |
LGTM |
Releasing today, so closing. |
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.