-
Notifications
You must be signed in to change notification settings - Fork 79
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
Interface newtypes #879
Interface newtypes #879
Conversation
keeping the value private as per https://www.howtocodeit.com/articles/ultimate-guide-rust-newtypes#newtype-essentials Formatting
getgrouplist doesn't like it.
Again, we're working with newtypes here, not the ids.
844173a
to
d1eee33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to do this! Amazing work already, but I think we can be a little more strict with getting the inner value of the different newtypes. There really shouldn't be a need to access the inner value except for interoperating with functions from libc (and the serialization/deserialization for the timestamp files). So I think naming those .get()
methods to something a little more scary sounding like .inner()
or .unwrap()
makes sense. I've also commented on a few places where I think we can replace getting the inner value with an alternative syntax that doesn't need to get the inner value.
(sorry for my late response in this thread by the way, I was away for a couple of weeks, I hope my feedback is still appreciated)
Always! I've learned some new things from both you and bjorn's feedback, so thanks for taking the time |
There are a lot of merge commits and 'fix' commits in this PR that I'd prefer to see squashed, I'll fiddle with the repo. |
Implementation for #769.
Throughout the code,
id.get()
is called. We could implementDeref
so we can*id
into the libc type. deref coercion makes this nice to work with.