Skip to content

refactor: Use regular functions rather than FromStr impls#8178

Merged
Hocuri merged 4 commits intomainfrom
hoc/no-more-from-str
Apr 27, 2026
Merged

refactor: Use regular functions rather than FromStr impls#8178
Hocuri merged 4 commits intomainfrom
hoc/no-more-from-str

Conversation

@Hocuri
Copy link
Copy Markdown
Collaborator

@Hocuri Hocuri commented Apr 26, 2026

Implementing FromStr and then calling parse() creates an
indirection, which is hard to follow for people who are not familiar
with Rust. @r10s recently voiced this problem when we were pair-programming, and I agree.

We can decide what exactly to call the new function.

I didn't remove all FromStr implementations yet; FromStr for Fingerprint, FromStr for Params, FromStr for MozConfigTag and FromStr for EphemeralTimer are still left.

Hocuri added 2 commits April 26, 2026 09:43
Implementing `FromStr` and then calling `parse()` creates an
indirection, which is hard to follow for people who are not familiar
with Rust. r10s recently had this problem.
@Hocuri Hocuri requested review from link2xt and r10s April 26, 2026 07:54
@Hocuri Hocuri force-pushed the hoc/no-more-from-str branch from 3b56774 to 04786f8 Compare April 26, 2026 08:37
@link2xt
Copy link
Copy Markdown
Collaborator

link2xt commented Apr 26, 2026

Note about not using parse() can go into https://github.com/chatmail/core/blob/main/STYLE.md
And maybe similar for .into() and .try_into(), better use Type::from and Type::try_from and not implement such traits internally. In simple cases like converting vectors to slices and i64 to u64 with failing if it does not convert it seems ok to use existing .into() and .try_into() implementations, but we can also decide to always use e.g. u64::try_from(rowid)? instead of .try_into().

I looked at try_into() usage, and this is particularly difficult to read:

core/src/configure.rs

Lines 522 to 524 in 287d730

let Ok(security) = params.socket.try_into() else {
return None;
};

The code actually called is here:

core/src/transport.rs

Lines 53 to 64 in 287d730

impl TryFrom<Socket> for ConnectionSecurity {
type Error = anyhow::Error;
fn try_from(socket: Socket) -> Result<Self> {
match socket {
Socket::Automatic => Err(format_err!("Socket security is not configured")),
Socket::Ssl => Ok(Self::Tls),
Socket::Starttls => Ok(Self::Starttls),
Socket::Plain => Ok(Self::Plain),
}
}
}

Implementing FromStr and directly using from_str is somewhat fine, but then we can as well implement it not as a trait and call it new().

@Hocuri Hocuri merged commit 0580056 into main Apr 27, 2026
31 checks passed
@Hocuri Hocuri deleted the hoc/no-more-from-str branch April 27, 2026 09:09
@r10s
Copy link
Copy Markdown
Contributor

r10s commented Apr 27, 2026

thanks a lot for that ❤️

Hocuri added a commit that referenced this pull request Apr 30, 2026
Follow-up to
#8178 (comment)

In a previous version, I added a note that the JsonRPC API is a notable exception, but I removed it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants