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.

Sign up for GitHub

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

Jump to bottom

Move common fields from MessageCommon to Message #946

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Move common fields from MessageCommon to Message #946

wants to merge 2 commits into from

Conversation

tar-xzf
Copy link

@tar-xzf tar-xzf commented Sep 28, 2023

@teloxidebot
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @WaffleLapkin (or someone else) soon.

@teloxidebot teloxidebot added C-core crate: teloxide-core C-main crate: teloxide S-waiting-on-review Status: Awaiting review from the assignee labels Sep 28, 2023
WaffleLapkin
WaffleLapkin reviewed Sep 29, 2023
View reviewed changes
crates/teloxide-core/src/types/message.rs
@@ -671,10 +669,7 @@ mod getters {
/// Returns the user who sent the message.
#[must_use]
pub fn from(&self) -> Option<&User> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you deprecate from (the function)? If it's just a field access I don't think we need it. Also the same for sender_chat.

(don't forget to update the changelog too)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's trivial to get rid of all usages of .from() except this one:

teloxide/crates/teloxide/src/dispatching/filter_ext.rs

Line 71 in d21ca11

(filter_from, Message::from),

Do you still want to deprecate it? If so, should I put #[allow(deprecated)] here?
It goes into user-facing rustdoc, so an anonymous or private function is not a good option here, I guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should remove it, I don't think filter_from makes much sense..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tar-xzf sorry, after re-reading this now I changed my mind >_>

filter_from still makes sense, given that it's an Option.

Please add it back. An #[allow(deprecated)] with a // FIXME: change macro so that we can filter things without getters should be fine.

@WaffleLapkin
Copy link
Member

@teloxidebot author

@teloxidebot teloxidebot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author and removed S-waiting-on-review Status: Awaiting review from the assignee labels Sep 29, 2023
@WaffleLapkin WaffleLapkin added this to the v0.13.0 milestone Oct 14, 2023
@WaffleLapkin
Copy link
Member

ping @tar-xzf, this is still waiting on you

@tar-xzf
Copy link
Author

tar-xzf commented Oct 15, 2023

@teloxidebot ready

@WaffleLapkin
Copy link
Member

Oh noooo, I did not notice the comment and @teloxidebot was not working properly at the moment D:

@WaffleLapkin WaffleLapkin added S-waiting-on-review Status: Awaiting review from the assignee and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author labels Jan 10, 2024
WaffleLapkin
WaffleLapkin requested changes Jan 10, 2024
View reviewed changes
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry again for the lond delay and back&forth changes 😓

Hopefully this should be the last thing.

crates/teloxide-core/src/types/message.rs
@@ -671,10 +669,7 @@ mod getters {
/// Returns the user who sent the message.
#[must_use]
pub fn from(&self) -> Option<&User> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tar-xzf sorry, after re-reading this now I changed my mind >_>

filter_from still makes sense, given that it's an Option.

Please add it back. An #[allow(deprecated)] with a // FIXME: change macro so that we can filter things without getters should be fine.

@teloxidebot teloxidebot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author and removed S-waiting-on-review Status: Awaiting review from the assignee labels Jan 10, 2024
@WaffleLapkin WaffleLapkin added the A-tba-types Area: representation of telegram bot API types label Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WaffleLapkin WaffleLapkin WaffleLapkin requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@WaffleLapkin WaffleLapkin

Labels
A-tba-types Area: representation of telegram bot API types C-core crate: teloxide-core C-main crate: teloxide S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author
Projects
None yet
Milestone
v0.13.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tar-xzf @teloxidebot @WaffleLapkin