Add GECOS field on UNIX systems.

[apognu]

This pull request adds the comment field (GECOS) that should be present on UNIX systems to the UserExtras struct as an Option<OsString>. The field would be None if the field is empty.

Refers to #12.

[OLEGSHA]

Hmm... Why is it a Option? How is an empty GECOS comment different from a missing GECOS comment? Why can with_gecos not be used to set the comment to None?

[apognu]

Good point, we don't need the Option and it will simplify the API to not have it. I will update the PR as soon as I can.

[apognu]

Done. Should I add some tests around this? I could not find any tests related to extras, but I can add them if you want.

[OLEGSHA]

There's a general lack of unit tests for the core functionality in the crate, and I think it'd be best to address this in a separate PR, For the sake of consistency I'd leave this PR as-is. I assume you've tested everything manually. Anyway, LGTM.

Erm... so I'm not really the maintainer, so you've got to wait for @gierens who seems to be running the place. Should take about a week from my experience.

[apognu]

Noted.

Yes, I have tested manually from different NSS providers, and things were fine. 👍 Could not test on BSD, though.

[gierens]

Hey guys, sorry for the late reply, been a little busy lately.

Thanks for the contribution, this looks good to me. The only thing that also was mnetioned would be testing. Sure, @OLEGSHA you are correct, there is a lack of tests ... but we have to start somewhere, so it would be great if you could add tests for this @apognu

[apognu]

Sure thing! I'll add a few by tomorrow and push again.

[apognu]

I added a few (mocked) tests, and edited the debug print one.

We are in the same situation, unfortunately, as with the other cases where I cannot assume a user on the system, or the current user, will have a comment specified (or even verify it). So short of refactoring the tests to use something like nss_wrapper (which I might be interested in looking at outside of this PR), or you see another way of testing for this, I do not know if I can do more.

[apognu]

I'll see what happens with #15. If it gets merged, I'll add mocked tests related to this PR as well.

[gierens]

Alright, #15 is merged, thanks again! And I took care of the minor merge conflict with the main branch. You should be good to go! :)

[apognu]

Oh I got my numbers mixed, I thought this one was merged, I can rebase this properly if you want.

[gierens]

If you want, got for it. But I think it's also fine the way it is. I'm not thaaaat nitpicky. At least not until the conventional commits workflow it merged haha ... just joking

[apognu]

Mmm, I think I messed up my rebase somehow, it pulled a commit from main where I don't think it should have.

EDIT: ok now it should merge cleanly. 🤞

[gierens]

Great! Thanks for your contribution. LGTM