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

Move xmldom parsing to internal interface #19

Merged
merged 3 commits into from
Feb 5, 2018
Merged

Move xmldom parsing to internal interface #19

merged 3 commits into from
Feb 5, 2018

Conversation

pgoldrbx
Copy link
Contributor

Fixes #18

Changes

  • Create an internal interface to wrap the xmldom parser.
  • Add unit tests for new interface (maintain all existing tests, despite some redundancy)
  • Update helpers tests to leverage this tested interface

Outstanding questions

  • Using this interface in the helpers tests simplifies things but adds some interdependency. This is arguably not a great decision. It's worth some debate.
  • The parse method could have a better name? parseXML or parseFromString (like the upstream library)
  • Should the interface be more thoroughly tested so we don't need to know/care about the upstream dependency? or is it ok to simply stub the xmldom methods and rely on expected behavior and output?

@pgoldrbx pgoldrbx self-assigned this Dec 15, 2017
try {
return parser.parseFromString(xml, 'text/xml');
} catch (e) {
console.warn(ERR_INVALID_XML);
Copy link
Contributor Author

@pgoldrbx pgoldrbx Dec 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice now that we're not logging the exception raised by the parser. However, I think this is better addressed in #14

Copy link
Contributor

@taveras taveras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pgoldrbx this LGTM!
i rebased this branch, fixed some lint errors, and updated the changelog.

@gautamarora gautamarora mentioned this pull request Feb 4, 2018
9 tasks
@pgoldrbx pgoldrbx merged commit a08a366 into CondeNast:master Feb 5, 2018
@pgoldrbx pgoldrbx deleted the feat/parse-xml-interface branch February 5, 2018 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants