Making xmlError js friendly.#3
Conversation
cdegalitt
left a comment
cdegalitt
left a comment
There was a problem hiding this comment.
Sorry, but as of now, I cannot merge this PR due to multiple questions and styling issues.
| * @param {string} sourcePath - path to xml document | ||
| * @returns The parsed Schema | ||
| */ | ||
| exports.parseXml = function(sourcePath, options) { |
There was a problem hiding this comment.
This is not necessary, since you can use the underlying libxmljs, which is exported from this library.
See: line 13 of the very same index.js.
| { | ||
| "name": "libxmljs2-xsd", | ||
| "version": "0.26.3", | ||
| "name": "ksys-libxmljs2-xsd", |
There was a problem hiding this comment.
I cannot accept your PR with a name change to the library
There was a problem hiding this comment.
I will create a new branch and change the name back to the original
|
|
||
| void set_string_field(Local<Object> obj, const char *name, const char *value) { | ||
| void set_string_field(Local<Object> obj, const char *name, const char *value) | ||
| { |
There was a problem hiding this comment.
I know there is no style guide yet, but if we are to enforce oldschool "c-style" single-lined brackets, it should be done throughout the whole codebase. For now, IMHO, we should keep as-is.
| Local<Value> err = | ||
| Exception::Error(Nan::New<String>(error->message).ToLocalChecked()); | ||
| Local<Object> out = Local<Object>::Cast(err); | ||
| Local<Object> out = Nan::New<Object>(); |
There was a problem hiding this comment.
Why wouldn't we want to create a JavaScript Error object ?
IMHO, it is useful since it indicates that this is an error/exception that could also be thrown, or logged with a proper stack trace.
|
closing this PR as wrong branch is being merged in. |
2 participants
With regards to #2