Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

set toString() to output formatted output #183

Merged
merged 1 commit into from Feb 11, 2013

Conversation

Projects
None yet
4 participants
Contributor

mgcrea commented Feb 7, 2013

See #169

Does this implementation looks OK to you? I'll happily change it if you had something else in mind.

Collaborator

defunctzombie commented Feb 7, 2013

I would default to formatted output.

In the c++ code I would assert the argument length and then just access the argument. Since you always pass a bool, that seems cleaner to me.

Contributor

mgcrea commented Feb 8, 2013

@shtylman Just rewrote the implementation & fixed unit tests. Does it looks OK to you?

Contributor

mgcrea commented Feb 8, 2013

Not sure where/how I should define the constant (maybe on the Document instance?).

I've reversed the logic as you suggested.

Collaborator

defunctzombie commented Feb 8, 2013

I think the logic is backwards with your typeof check. Also, you can just do formatted === undefined versus typeof.

Contributor

mgcrea commented Feb 8, 2013

@shtylman yep, forgot to node-gyp rebuild, just fixed it.

Collaborator

defunctzombie commented Feb 8, 2013

Ok, Please squash the commits into one so I can review the entire changeset and merge as a single commit.

Contributor

mgcrea commented Feb 8, 2013

@shtylman done

Collaborator

defunctzombie commented Feb 8, 2013

@polotek @ncb000gt if there are no objections I will land this tonight.

defunctzombie added a commit that referenced this pull request Feb 11, 2013

Merge pull request #183 from mgcrea/master
set toString() to output formatted output

@defunctzombie defunctzombie merged commit 7a30306 into libxmljs:master Feb 11, 2013

1 check passed

default The Travis build passed
Details
Collaborator

defunctzombie commented Feb 11, 2013

Published as version 0.7.0 Thanks :)

Minor version was bumped due to break from previous toString output.

Collaborator

ncb000gt commented Feb 11, 2013

Sorry, I didn't get around to looking at this before. I was shoveling out from under 2+ feet of snow. However, looks good to me. :) 👍

Collaborator

defunctzombie commented Feb 11, 2013

@ncb000gt nice! We got ~11 inches in NYC.

igalep commented Feb 11, 2013

Hi,
I've been trying to use the updated ver of "libxmljs" with toString() method.

This is the code examp I'm executing :

var d = libxmljs.Document();
d.node("a");
var r = d.root();
r.node("newNode1", "blablabla").attr({foo: '0000'});
var aa = r.find("newNode1");
aa[0].node("child", "blablabla").attr({foo: '0000'});
r.node("newNode2", "blablabla").attr({foo: '0000'});

console.log(d.toString());

and the output looks like :

blablablablablabla blablabla

As you see, the second added node ("child") is not printed where it should be originally located.

Could you please check this issue ?

Thanks.

Collaborator

defunctzombie commented Feb 11, 2013

@igalep Please post a new issue on this topic versus adding to this closed issue. Also, if you could put the xml into code blocks, the output will be easier to read. And please set the text values to something easier to discern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment