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

Add System.username #12605

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

will
Copy link
Contributor

@will will commented Oct 12, 2022

In discussion of #12604 we determined that a useful shortcut would be to grab the current username directly, which avoids allocating several strings out of the lib passwd struct and the System::User class if the only thing one cares about is the current user name.

Reference #7738

@HertzDevil
Copy link
Contributor

HertzDevil commented Oct 13, 2022

The Windows equivalent is GetUserNameExW but note that a few different formats are available. (IIRC only two or three work on a Windows Home installation) Also it is worth considering whether $USER or %USERNAME% could override this return value.

You might notice that Path.home does a very similar thing, it's up to you whether you want to factor out the code for getpwuid_r.

Personally I prefer it being a class method of just System, rather than System::User.

@will
Copy link
Contributor Author

will commented Oct 13, 2022

Also it is worth considering whether $USER or %USERNAME% could override this return value.

For this part, my thinking was to have the stdlib do the harder thing of making system calls, and leave the choice of checking env vars up to user code, if the situation should or shouldn't respect env vars being different than the result of getuid or GetUserNameExW.

@will
Copy link
Contributor Author

will commented Oct 13, 2022

Personally I prefer it being a class method of just System, rather than System::User.

I pushed two fixup commits to do this, one to just do it, and another to refactor out the duplicate code.

I left the windows part out for myself, since I don't have a way to test that at the moment, and while I could make a VM or something, it'll realistically be several weeks before I could find time to get that all working, so maybe someone else could pick that part up?

@will will changed the title Add System::User.current_user_name Add System.current_user_name Oct 13, 2022
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

I'd suggest to call this method System.username. It's more concise and follows the same scheme as System.hostname.

spec/std/system_spec.cr Outdated Show resolved Hide resolved
src/crystal/system/unix/path.cr Outdated Show resolved Hide resolved
src/system/user.cr Outdated Show resolved Hide resolved
@asterite
Copy link
Member

I'd suggest to call this method System.username. It's more concise and follows the same scheme as System.hostname.

Can you have multiple hostnames configured in a system, and a current one being used? Can you have multiple usernames in a system and a current one being used?

@HertzDevil
Copy link
Contributor

On POSIX there are the differences between the real user, the effective user, and the login user ($LOGNAME). Windows probably has similar differences.

@will
Copy link
Contributor Author

will commented Oct 13, 2022

I'd suggest to call this method System.username.

I made this change in the last fixup

edit: made a mistake with git, one second fixed

@will will changed the title Add System.current_user_name Add System.username Oct 13, 2022
Comment on lines +4 to +7
String.new(pwd.pw_name)
else
raise RuntimeError.from_errno("Could not get current user name")
end
Copy link
Contributor

@Sija Sija Oct 14, 2022

Choose a reason for hiding this comment

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

I'd suggest using an early-return.

Suggested change
String.new(pwd.pw_name)
else
raise RuntimeError.from_errno("Could not get current user name")
end
return String.new(pwd.pw_name)
end
raise RuntimeError.from_errno("Could not get current user name")

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

Successfully merging this pull request may close these issues.

6 participants