Implement System::User on Windows#14933
Conversation
HertzDevil
added
kind:feature
platform:windows
| end | ||
|
|
||
| private def self.from_sid(sid : LibC::SID*) : ::System::User? | ||
| canonical = sid_to_name(sid) || return |
There was a problem hiding this comment.
| canonical = sid_to_name(sid) || return | |
| return unless canonical = sid_to_name(sid) |
There was a problem hiding this comment.
@straight-shoota Why thumbs down? It's an established pattern, used as well one line below...
There was a problem hiding this comment.
No, this is not an established pattern. And it never will be (if it's up to me).
In a trailing conditional, the condition should always be a predicate without side effects (like canonical.type.sid_type_user?).
Assignments should never be in a trailing condition.
The current style is concise and expressive. It focuses on the main action, an assignment to a local variable. The value that we want to assign comes from a method call and if it is unavailable, we return. That's very readable. Your suggestion is not. It hides the fact that this is an assignment behind the less significant control flow action for error handling.
There was a problem hiding this comment.
TBH I write return unless x = y in all my crystal code. I like that the return statement is explicit, and not buried at the end of an expression (especially below).
I also prefer a single style over a mix of them.
| end | ||
|
|
||
| private def self.lookup_primary_group_id(name : String, domain : String) : String? | ||
| domain_sid = name_to_sid(domain) || return |
There was a problem hiding this comment.
| domain_sid = name_to_sid(domain) || return | |
| return unless domain_sid = name_to_sid(domain) |
|
I noticed the Go implementation has quite some helpful code comments that explain how the code works and why. |
4 participants
Depends on #14929.
This is for the most part a straight port of Go's implementation, including their interpretation of primary groups on Windows (as opposed to whatever Cygwin does).