more closely match c xattr methods #3

Merged
merged 10 commits into from Jan 15, 2013

Conversation

Projects
None yet
2 participants
Contributor

cinterloper commented Dec 5, 2012

I have changed the default list to only read the names of the xattrs, not to get their data as well.
This will result in less io overhead, and more closely matches the underlying api.

I have left the old list as 'glist', and added 'remove' and 'get' functions.

if you do, please pull current, fixed an important mistake

Owner

dmachi commented Jan 3, 2013

Thanks for these updates, I'll give them a look through and merge.

I might prefer to leave the current api as is (despite not matching the underlying api) and make the functionality you added as a newly named method. While I do appreciate that what you have done more closely matches the underlying API and the functionality is usefull, It is also common (at least in my use cases) to grab all the names and data as an object, so thats why that functionality was combined.

In any case, I think I can merge the functionality in and will think about the other piece (and see how much code is already dependent on that behavior).

Contributor

cinterloper commented Jan 12, 2013

Hey, please review my latest commit and tell me what you think.
I left your original functionality in place by default and added a toggle for strict c-api compatibility.

I strongly believe this will be important from a performance point of view in cases where there are large directories with lots of xattr data

thx

----- Original Message -----
From: "Dustin Machi" notifications@github.com
To: "dmachi/node-xattr" node-xattr@noreply.github.com
Cc: "cinterloper" grant@iowntheinter.net
Sent: Thursday, January 3, 2013 11:39:43 AM
Subject: Re: [node-xattr] more closely match c xattr methods (#3)

Thanks for these updates, I'll give them a look through and merge.

I might prefer to leave the current api as is (despite not matching the underlying api) and make the functionality you added as a newly named method. While I do appreciate that what you have done more closely matches the underlying API and the functionality is usefull, It is also common (at least in my use cases) to grab all the names and data as an object, so thats why that functionality was combined.

In any case, I think I can merge the functionality in and will think about the other piece (and see how much code is already dependent on that behavior).


Reply to this email directly or view it on GitHub .

Owner

dmachi commented Jan 15, 2013

This works for me. I have no doubt that there are cases where just retrieving the xattr names will be better, and so I appreciate the patch.

Of course if you are going to end up retrieving both values anyway, then it is probably counterproductive. I believe this is why more recent versions of python abandoned the api that followed the underlying c api strictly.

In any case, with this commit, we'll be able to support both situations.

dmachi pushed a commit that referenced this pull request Jan 15, 2013

Merge pull request #3 from cinterloper/master
more closely match c xattr methods

@dmachi dmachi merged commit 3e00e5a into dmachi:master Jan 15, 2013

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