Skip to content

Move common fields from MessageCommon to Message #946

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
Closed
tar-xzf wants to merge 2 commits into from
Closed

Move common fields from MessageCommon to Message #946

tar-xzf wants to merge 2 commits into from

Conversation

@tar-xzf
Copy link

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

@teloxidebot
Copy link
Collaborator

teloxidebot commented Sep 28, 2023

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
impl Message {
/// Returns the user who sent the message.
#[must_use]
pub fn from(&self) -> Option<&User> {
Copy link
Contributor

@WaffleLapkin WaffleLapkin Sep 29, 2023

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

@tar-xzf tar-xzf Sep 30, 2023

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
Contributor

@WaffleLapkin WaffleLapkin Oct 3, 2023

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
Contributor

@WaffleLapkin WaffleLapkin Jan 10, 2024

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
Contributor

WaffleLapkin commented Sep 29, 2023

@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
Contributor

WaffleLapkin commented Oct 14, 2023

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
Contributor

WaffleLapkin commented Jan 10, 2024

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 suggested changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@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
impl Message {
/// Returns the user who sent the message.
#[must_use]
pub fn from(&self) -> Option<&User> {
Copy link
Contributor

@WaffleLapkin WaffleLapkin Jan 10, 2024

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
@syrtcevvi syrtcevvi mentioned this pull request Jul 20, 2024
Merged
@WaffleLapkin
Copy link
Contributor

WaffleLapkin commented Aug 14, 2024

r? syrtcevvi

@syrtcevvi syrtcevvi closed this Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@WaffleLapkin WaffleLapkin WaffleLapkin requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@syrtcevvi syrtcevvi

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.

4 participants

@tar-xzf @teloxidebot @WaffleLapkin @syrtcevvi