-
Notifications
You must be signed in to change notification settings - Fork 157
General improvements #367
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
General improvements #367
Conversation
longer than i would like due to a lack of public helpers from `sqlite`.
…:types::ChannelKind`
…::convert::TryFrom` impls
| #[allow(unsafe_code)] | ||
| let result_code = unsafe { sqlite::ffi::sqlite3_db_cacheflush(raw_connection) }; |
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.
If we're sticking with sqlite we should consider upstreaming this, because it'd be very unfortunate to keep unsafe code inside grammers.
Although, I'm not particularly tied to sqlite. All I wanted is an offline local database which sqlite fits (and I'm used to from Telethon). But I'd be happy with pure-Rust alternatives like prsqlite or Limbo.
| } | ||
|
|
||
| pub(crate) fn peer_ref(&self) -> PeerRef { | ||
| pub fn peer_ref(&self) -> PeerRef { |
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.
Yeah fair, I wanted to revisit these and make sure they were consistent at some point.
I think we'll want peer and peer_ref on pretty much everything that contains a peer (and sometimes sender and sender_ref).
| pub fn peer_id(&self) -> PeerId { | ||
| self.peer_ref().id | ||
| } |
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.
Good.
| /// When this timeout occurs, | ||
| /// the client will attempt to fetch updates by itself, | ||
| /// ignoring all the updates that arrive in the meantime. | ||
| /// After all updates are fetched when this happens, | ||
| /// the client will resume normal operation, | ||
| /// and the timeout will reset. |
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.
I'd probably drop this commit entirely.
I'm OK with noisy diffs on doc-comments, since any changes there need to make sense and be read as a whole IMO. (Also a good opportunity to recheck that they still make sense.)
I mostly go by gut feeling on these. Maybe I should embrace this harder rule of breaking on comma or period.
| /// Value used for a channel with its [`tl::types::Channel::broadcast`] flag set to `true`. | ||
| Broadcast, |
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.
Broadcast and Gigagroup were together because a Gigagroup is like a megagroup where noone can talk (a broadcast).
But, I'm fine with either. In a way Broadcast was indeed the "original" type, then came Megagroup, and then Gigagroup.
|
|
||
| Enables the user to use HTML text to send formatted messages. | ||
|
|
||
| ## snafu |
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.
Wish I had an overview on all error crates and which ones were good for libraries, because I frankly cannot remember. This one rings a bell so it might be it.
| <Self as From<&tl::types::PeerChannel>>::from(&channel) | ||
| } | ||
| } | ||
| impl<'a> From<&'a tl::types::PeerChannel> for PeerId { |
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.
These are nice to have (and I can see why people would use macros), but it's again unfortunate this only works for one reference type.
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.
What other types are you referring to by “only one reference type”?
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.
Box and other smart pointers.
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.
That's what Deref is for.
| <Self as From<&PeerRef>>::from(&peer) | ||
| } | ||
| } | ||
| impl<'a> From<&'a PeerRef> for tl::enums::InputChannel { |
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.
Yeah damn this file and commit overall is huge. Wonder if macros would help or be worse.
| } | ||
|
|
||
| /// Useful information about this peer. | ||
| pub fn info(&self) -> PeerInfo { |
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.
I'd be wary about introducing a method like this because it could be very easily confused with a channel's description / about / bio / whatever Telegram introduces.
I think the From conversion is enough.
| } | ||
|
|
||
| #[inline] | ||
| pub fn r#ref(&self) -> PeerRef { |
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.
I see why we wouldn't name this peer_ref but it's really unfortunate that we need the raw identifier syntax (is this why we talked about PeerCap)?
Maybe the From is enough.
Or is there value in providing a named getter like this? Wouldn't it then be better as to_ref? (Or to_peer_ref.) Though I guess to_ implies expensive even though this is not.
No description provided.