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

alternative way of getting volume name on Mac OS X <10.7 #3

Closed
wants to merge 1 commit into from

Conversation

dliessi
Copy link
Contributor

@dliessi dliessi commented Apr 23, 2014

kCFURLVolumeNameKey was introduced in 10.7.

@dliessi
Copy link
Contributor Author

dliessi commented Apr 23, 2014

This builds without warnings on 10.6, and mocha reports 3 passing.
Can you test if it works as usual on your machine?

@dliessi
Copy link
Contributor Author

dliessi commented Apr 23, 2014

Forgot to say that I used CXX=clang++-mp-3.4 node-gyp -v rebuild (clang++-mp-3.4 is Clang 3.4 provided by MacPorts).

desc = MYCFStringGetV8String(CFErrorCopyDescription(error));
#else
CFStringRef errorString = CFSTR("Could not get volume name from FSRef.");
desc = MYCFStringGetV8String(errorString);
Copy link
Owner

Choose a reason for hiding this comment

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

desc = String::New("Could not get volume name from FSRef.")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, yes.
Should I add a new commit to the branch or --amend this one?

Copy link
Owner

Choose a reason for hiding this comment

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

--amend would be preferred

@LinusU
Copy link
Owner

LinusU commented Apr 23, 2014

I'll have a look at this later tonight, but it looks good

kCFURLVolumeNameKey was introduced in 10.7.
@LinusU
Copy link
Owner

LinusU commented Apr 24, 2014

Sorry, some things got in the way yesterday. I'll do it tonight.

@LinusU
Copy link
Owner

LinusU commented Apr 24, 2014

I wrote this into a separate file, with better error reporting, and added some tests. Could you please check out my master and test if it works okay.

I'll add you as a contributor to the package.json if you want to. Name, email, url?

@dliessi
Copy link
Contributor Author

dliessi commented Apr 25, 2014

Builds without warnings, passes 3 mocha tests and fails the volume name test, but this is expected, since my root volume name is not Macintosh HD; the real volume name is correctly determined and shown in the error message.

I'll add you as a contributor to the package.json if you want to. Name, email, url?

That would be nice, thanks!
I'm Davide Liessi, davide.liessi AT gmail.com.

@LinusU
Copy link
Owner

LinusU commented Apr 25, 2014

Yeah, I didn't really know what to do with the volume name. But that's great, I'll push a release to npm later tonight. Thanks for the help!

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