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

os: add .homedir() #1670

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@robertkowalski
Member

robertkowalski commented May 10, 2015

In almost every cli-client that I write I need the current homedir
at a certain point.

While we have a function for getting the directory for storing
temporary files (callled tmpdir), a function for receiving the
location of the home directory is still missing.

os: add .homedir()
In almost every cli-client that I write I need the current homedir
at a certain point.

While we have a function for getting the directory for storing
temporary files (callled `tmpdir`), a function for receiving the
location of the home directory is still missing.
@trevnorris

This comment has been minimized.

Contributor

trevnorris commented May 10, 2015

What happens if process.env.{HOME,USERPROFILE} isn't set?

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 10, 2015

I think this could be useful if there is a reliable and robust implementation.

@fivdi

This comment has been minimized.

fivdi commented May 10, 2015

See also user-home

@mscdex mscdex added the os label May 10, 2015

@robertkowalski

This comment has been minimized.

Member

robertkowalski commented May 10, 2015

oh good points, i just took a look at user-home

maybe it makes sense to take the code from https://github.com/sindresorhus/user-home/blob/master/index.js and add a notice t our LICENSE file (it is MIT)

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 10, 2015

Then again, if userland can do this fine, why should it be in core?

@domenic

This comment has been minimized.

Member

domenic commented May 10, 2015

I am definitely +1 on including something like this, although indeed it seems this might not be robust enough of an implementation.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented May 10, 2015

Should it not throw an Error instead of returning null or undefined (user-home returns null, your code returns undefined) when the home directory couldn't be determined?

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 10, 2015

I don't think this should be done using environment variables. There are existing C APIs for this, right?

@vkurchatkin

This comment has been minimized.

Member

vkurchatkin commented May 10, 2015

prior discussion: #682

@sindresorhus

This comment has been minimized.

sindresorhus commented May 10, 2015

I strongly feel this should have a robust implementation in libuv instead of just checking process.env => libuv/libuv#11

The ecosystem use the home dir for so many different things, but the current userland ways of inferring it are just too fragile.

@rvagg

This comment has been minimized.

Member

rvagg commented May 11, 2015

As per the previous discussion in #682 (my comment specifically: #682 (comment)) and echoing @sindresorhus' comment above; I'm -1 on this unless we can come up with a solid cross-platform implementation that doesn't involve a lot of guesswork. The main reason the previous 2 persistent-history for REPL PRs didn't make it in is because they came bundled with implementations like this and @chrisdickinson found a good compromise of just removing the need for a homedir completely--it'd be nice if we could move to using a homedir but the current solution(s) are unsatisfactory. When we expose an API like this to users we are suggesting that it's reliable, but it's absolutely not (sadly!).

@piscisaureus

This comment has been minimized.

Member

piscisaureus commented May 11, 2015

I think an API to retrieve the home directory would be an acceptable addition to libuv.

The implementation is also not that hard - on unix the proper way would be to to call getpwnam(3) and return the pw_dir member from the returned struct passwd*.

On Windows the implementation would use SHGetKnowFolderPath and to retrieve the FOLDERID_Profile value.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 11, 2015

@piscisaureus I have already done that. Would you mind looking over https://github.com/cjihrig/userinfo and letting me know if that would be suitable for libuv?

@piscisaureus

This comment has been minimized.

Member

piscisaureus commented May 11, 2015

@cjihrig

That looks pretty good in general.
I could comment a bit (use GetUsernameW), but reviewing it in depth makes little sense since it's not a libuv patch.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 11, 2015

OK, thanks. I'll work on something for libuv.

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented May 11, 2015

@piscisaureus What if the home directory doesn't exist?

@domenic

This comment has been minimized.

Member

domenic commented May 11, 2015

Well I think the precedent for other os APIs has been to return 0 in that case ... ;)

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 21, 2015

Landed in libuv: libuv/libuv@a62c2d5 \o/

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 21, 2015

I'll work on the io.js bits soon and PR it once it's in deps/libuv.

@Fishrock123 Fishrock123 added the libuv label May 21, 2015

@robertkowalski

This comment has been minimized.

Member

robertkowalski commented May 21, 2015

i'll just keep this open for reference / further discussion, please close it once @cjihrig has a superseeding PR

@cjihrig cjihrig referenced this pull request May 25, 2015

Merged

os: add homedir() #1791

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 25, 2015

Closing in favor of #1791

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