Drop syn proc & quote dep & other id128 refactor #79
Conversation
| } | ||
| } | ||
|
|
||
| /// Return this machine unique ID. | ||
| #[deprecated(since = "0.3.2", note = "use Id128::from_machine()")] |
There was a problem hiding this comment.
This looks like an arbitrary/cosmetic rename. If so, let's not do that. The current naming may look ugly, but it is meant to stick closer to the C sd_id128_* APIs.
There was a problem hiding this comment.
The current naming may look ugly, but it is meant to stick closer to the C
sd_id128_*APIs.
Yeah that was my understanding reading the code too. It's just that the C API is... for C so since this is a Rust lib it should follow rust API guidelines. Keeping C API is confusing
There was a problem hiding this comment.
True... to a certain extent. Keeping some alignment on the names helps a lot with discoverability, that's what I've see from other people/consumers using it.
My rule of thumb is roughly to keep (very similar / identical) names for things that are basically 1:1 ports, and exercise a bit more freedom for things that do not fall in that bucket. Here the former applies though.
There was a problem hiding this comment.
Then we can keep the name (get_machine()) but still stick it onto the Id128 struct. By not having free functions we can in 0.4 hide the id128 module and just reexport the Id128 struct. API ref
0.3: libsystemd::id128::get_machine()
0.4 libsystemd::Id128::get_machine()
Looking at https://github.com/coreos/zincati/blob/10a9c409013364697146ff5bf43c8bdaede347ae/src/identity/mod.rs having the id128 module makes the code much uglier than if all the functions were on the Id128 struct.
|
Thanks for the PR. I think it is generally nice, but at the same time I think it got a bit out of hand and dragged in a lot of misc stuff. |
| } | ||
|
|
||
| /// Return the unique boot ID. | ||
| pub fn from_boot() -> Result<Self, SdError> { |
There was a problem hiding this comment.
As much as we may not like the original name, this is a port of sd_id128_get_boot(3) so it would be better to align the naming on that.
There was a problem hiding this comment.
Calling it get_boot would diverge from the Rust method naming scheme. It could just be called boot instead, but from_boot is what the systemd crate named it.
There was a problem hiding this comment.
Indeed that's unfortunate, naming things is hard and aligning multiple beasts even harder. We cowardly refuse to walk into the minefield here and just go with that upstream has, see other comment for the rationale.
derivefeature from serde & manually impl Error instead of with thiserrortry_from_slicein favour ofTryFrom<[u8]>parse_strin favor ofFromStrId128::from_bootmethodget_machinein favor ofId128::from_machineget_machine_app_specificsince it goes against the rust API guidelines (users should callId128::from_machine()?.app_specific()instead)as_bytesmethodAsRef<[u8]>,AsRef<OsStr>From<[u8; 16]>,Copy,Default&Hash^x.y.ztox.y.zas cargo interprets them the sameThis PR is to the best of my knowledge backwards compatible
Closes #78