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

Making xmlError js friendly. #3

Closed
wants to merge 4 commits into from
Closed

Conversation

gotama
Copy link

@gotama gotama commented Sep 17, 2020

With regards to #2

Copy link
Owner

@cdegalitt cdegalitt left a comment

Choose a reason for hiding this comment

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

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) {
Copy link
Owner

Choose a reason for hiding this comment

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

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.

@@ -1,6 +1,6 @@
{
"name": "libxmljs2-xsd",
"version": "0.26.3",
"name": "ksys-libxmljs2-xsd",
Copy link
Owner

Choose a reason for hiding this comment

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

I cannot accept your PR with a name change to the library

Copy link
Author

Choose a reason for hiding this comment

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

I will create a new branch and change the name back to the original

@@ -5,28 +5,30 @@

using namespace v8;

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)
{
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Yes agree, will revert

Local<Value> err =
Exception::Error(Nan::New<String>(error->message).ToLocalChecked());
Local<Object> out = Local<Object>::Cast(err);
Local<Object> out = Nan::New<Object>();
Copy link
Owner

Choose a reason for hiding this comment

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

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.

@gotama
Copy link
Author

gotama commented Oct 29, 2020

closing this PR as wrong branch is being merged in.

#5

@gotama gotama closed this Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants