Skip to content
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

Remove sentinel values in names #292

Merged
merged 5 commits into from
Nov 23, 2024

Conversation

rsxrwscjpzdzwpxaujrr
Copy link
Contributor

Functions Chat::name, Chat::User::first_name, and Chat::Group::title have sentinel values, occasionally returning empty strings, which is a bad code practice. The docs also don't mention all the cases when the strings are going to be empty.

Telegram doesn't include all sender information because it assumes you already have it.
Originally posted by @Lonami in #212 (comment)

This creates confusion, leading to faulty code, as in #274 for example. Using Option will make it so users can't accidentally forget to check if the returned string is empty.

@Lonami
Copy link
Owner

Lonami commented Nov 22, 2024

My thinking here was that String already has an empty state, and the None vs String::default() distinction isn't meaningful, because Telegram does not return or allow setting empty strings as names.

But it is true that the Option makes it far more explicit, although also more annoying in the common (?) case.

@rsxrwscjpzdzwpxaujrr
Copy link
Contributor Author

It's more annoying to use Option when you don't check if the returned string is empty, which is the case for examples and tests, as none of them ever check, which is a buggy code (and btw that's a good example why sentinel values are bad). echo.rs prints "Responding to " without a name most of the time. Telegram not sending names when you already have them basically means the common case is when it doesn't send the name, so real-world applications must have some kind of cache of the names and check if the string is empty every time. And when you do check, it is becoming more annoying to have &str with sentinel values than Option<&str>, as you don't have all the sugar that Option provides and need to do something like chat.name().is_empty().not().then(|| chat.name()) to be able to, for example, match on it.

The meaning of None vs String::default() is to make it more convenient to write good code and less convenient to write bad code. Using empty strings as sentinel values resembles the usage of null values ("hey, a memory address can't be 0, let's use it when we don't have the value") and is not rust-way.

The ultimate one to blame for the annoyance here is Telegram not sending the names with every message.

@Lonami Lonami merged commit b733d70 into Lonami:master Nov 23, 2024
1 check passed
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.

2 participants