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 Etc module with methods for fetching user information #2368

Closed
wants to merge 3 commits into from

Conversation

tmatilai
Copy link

¡Hola!

Would you like to include Ruby inspired Etc module to the standard library? Or do I publish it as shard?

Disclaimer: I just started playing with the cool new language today, so might have some funny things in it.

@tmatilai tmatilai force-pushed the feature/etc_module branch from 31f7009 to 357da24 Compare March 25, 2016 23:18
property dir
property shell

def initialize(pwd : LibC::Passwd)
Copy link
Member

Choose a reason for hiding this comment

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

I would say that LibC::Passwd should not appear here. The wrapper module should perform the conversion IMO.

Also, take a look at http://crystal-lang.org/api/toplevel.html#record%28name%2C%2Aproperties%29-macro you will probably like it.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. Naming is hard, but I pushed a new commit trying to fix that. (As a separate commit for clarity, will squash it later.)

fun getlogin : Char*
fun getpwnam(name : Char*) : Passwd*
fun getpwuid(uid : UInt32) : Passwd*
fun getuid : UInt32
Copy link
Author

Choose a reason for hiding this comment

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

getuid should propably be defined somewhere else? In process.cs?

@asterite
Copy link
Member

Hola!

Thank you for this. We have to analyze this a bit. It's way too unix specific. What would happen in Windows, this module won't be available? I think the standard library shouldn't have too platform-specific functionality, but I don't know.

@tmatilai
Copy link
Author

Seems that in Ruby the non-existing methods just return nil. And getlogin on Windows returns value of USER or USERNAME environment variable. And anyway, in Ruby the module has a couple of other methods too.

Your call, for me this was just a fun exercise. =)

@jhass
Copy link
Member

jhass commented May 29, 2016

If we want this it should go to the new System module and avoid prefixes such as get.

@asterite
Copy link
Member

Yes, having this in System sounds good. I'd check with @ysbaddaden to generate the correct bindings, though.

@ysbaddaden
Copy link
Contributor

Bindings for getlogin and related syscalls should already be available in posix, they just need to be enabled for Crystal, then build targets again.

@miketheman
Copy link
Contributor

#codetriage
So it appears that this was left in a state where the following seem to be the path forward:

  • enable bindings for relevant syscalls by submitting & merging a PR to https://github.com/ysbaddaden/posix/
  • Do something with the change (uncertain of what this is)
  • Add some of the code & specs from this PR under the System module without get prefix, so System.login would return miketheman on my system
  • Profit!!

If that's correct and desirable, I'm happy to take a stab at this, but I'm uncertain of what Step 2 would be.
@ysbaddaden the README on the posix repo isn't very clear as to what happens next with changes there.

miketheman added a commit to miketheman/crystal that referenced this pull request Aug 6, 2017
Uses current conventions.

Split out from crystal-lang#2368

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@miketheman miketheman mentioned this pull request Aug 6, 2017
miketheman added a commit to miketheman/crystal that referenced this pull request Aug 6, 2017
Uses current conventions.

Split out from crystal-lang#2368

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@straight-shoota
Copy link
Member

It seems like this PR is mostly superseeded by the more recent #5627 and #4800

@RX14 RX14 closed this Apr 5, 2018
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.

None yet

9 participants