Add migrate_chat_data method to dispatcher #2848

DonalDuck004 · 2022-01-06T15:27:38Z

@Bibo-Joshi now there should be no problem, do I have to create a unit_test next?

Bibo-Joshi

Thanks for the RP! The code looks good overall - my comments are largely nitpicks :)

Yes, we will need unit tests. You can place them into tests/test_dispatcher.py. Please be sure that the tests cover all cases, e.g. passing a message without migration data, passing both message & old/new_chat_id etc …
The contribution guide has info on how to run the tests locally.
At the bottom of this PR you see the results of the test suite - the "codecov" results tell you if you have covered all changed lines in your unit tests.

Additionally, the pre-commit test failed. Please be sure to install the pre-commit hooks as described in the contrib guide. you can also run them manually with pre-commit run after git add -ing your changes.

telegram/ext/_dispatcher.py

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

DonalDuck004 · 2022-01-07T14:11:00Z

Thanks for the RP! The code looks good overall - my comments are largely nitpicks :)

Yes, we will need unit tests. You can place them into tests/test_dispatcher.py. Please be sure that the tests cover all cases, e.g. passing a message without migration data, passing both message & old/new_chat_id etc … The contribution guide has info on how to run the tests locally. At the bottom of this PR you see the results of the test suite - the "codecov" results tell you if you have covered all changed lines in your unit tests.

Additionally, the pre-commit test failed. Please be sure to install the pre-commit hooks as described in the contrib guide. you can also run them manually with pre-commit run after git add -ing your changes.

Ok, I'll try tonight/tomorrow

DonalDuck004 · 2022-01-08T14:49:59Z

Thanks for the RP! The code looks good overall - my comments are largely nitpicks :)

Yes, we will need unit tests. You can place them into tests/test_dispatcher.py. Please be sure that the tests cover all cases, e.g. passing a message without migration data, passing both message & old/new_chat_id etc … The contribution guide has info on how to run the tests locally. At the bottom of this PR you see the results of the test suite - the "codecov" results tell you if you have covered all changed lines in your unit tests.

Additionally, the pre-commit test failed. Please be sure to install the pre-commit hooks as described in the contrib guide. you can also run them manually with pre-commit run after git add -ing your changes.

Both my test aren't written correctly, but I haven't in mind any other way to make it, can you help me? :/

Bibo-Joshi

In general, what the tests should do is:

Store some data in dispatcher.chat_data[some_id]
pass arguments to migrate_chat_data
if you expect that to raise an exception, check that it was raised
otherwise, check that dispatcher.chat_data was adjusted as expected

Maybe this helps a bit. Also, here you can see which lines are not covered by the tests.

Feel free to ping me in https://t.me/pythontelegrambotdev, if you have any question :) (@BiboJoshi on TG)

tests/test_dispatcher.py

DonalDuck004 · 2022-01-08T22:34:35Z

In general, what the tests should do is:

Store some data in dispatcher.chat_data[some_id]

pass arguments to migrate_chat_data

if you expect that to raise an exception, check that it was raised

otherwise, check that dispatcher.chat_data was adjusted as expected

Maybe this helps a bit. Also, here you can see which lines are not covered by the tests.

Feel free to ping me in https://t.me/pythontelegrambotdev, if you have any question :) (@BiboJoshi on TG)

Thanks, for your time and help, now all tests pass except deep source, but I haven't changed any of the reported files, what should I do?

Bibo-Joshi

Thanks for the updates! LGTM except for my minor comments below :)
@Poolitzer, could you give it a second review?

DeepSource is currently reporting some false positives. I have already reported them, but they haven't updated it yet. We can ignore that for now.

tests/test_dispatcher.py

telegram/ext/_dispatcher.py

tests/test_dispatcher.py

Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>

Poolitzer

Code looks good.

telegram/ext/_dispatcher.py

# Conflicts: # telegram/ext/_dispatcher.py

Bibo-Joshi · 2022-01-19T20:32:27Z

I updated the PR according to #2852 and added an additional test. merging.
Thanks @DonalDuck004 for the nice contribution 👍

DonalDuck004 added 3 commits January 6, 2022 16:17

Add migrate_chat_data method to dispatcher

54e06f2

Update _dispatcher.py

90da782

Update _dispatcher.py

0247cbc

Bibo-Joshi requested changes Jan 7, 2022

View reviewed changes

DonalDuck004 and others added 5 commits January 7, 2022 14:58

Update telegram/ext/_dispatcher.py

b48de82

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

Update telegram/ext/_dispatcher.py

2ee7118

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

Update telegram/ext/_dispatcher.py

7cd1b7d

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

Update telegram/ext/_dispatcher.py

dd72bbb

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

Update _dispatcher.py

9bf29fa

DonalDuck004 added 4 commits January 8, 2022 14:46

Update _dispatcher.py

632e53a

Add test_migrate_chat_data to tests/test_dispatcher.py

9c85818

Update test_dispatcher.py

54dd1dd

Update test_dispatcher.py

c32e1f4

Bibo-Joshi reviewed Jan 8, 2022

View reviewed changes

DonalDuck004 added 5 commits January 8, 2022 21:36

Update test_dispatcher.py

f6ebf7f

Update test_dispatcher.py

6ea2998

Update test_dispatcher.py

eac170a

Update test_dispatcher.py

e5887a4

Update test_dispatcher.py

abb7e7f

Bibo-Joshi reviewed Jan 9, 2022

View reviewed changes

tests/test_dispatcher.py Outdated Show resolved Hide resolved

telegram/ext/_dispatcher.py Show resolved Hide resolved

harshil21 requested changes Jan 9, 2022

View reviewed changes

telegram/ext/_dispatcher.py Outdated Show resolved Hide resolved

telegram/ext/_dispatcher.py Outdated Show resolved Hide resolved

tests/test_dispatcher.py Outdated Show resolved Hide resolved

Apply suggestions from code review

f24ad71

Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>

harshil21 added this to the v14 milestone Jan 10, 2022

harshil21 added the enhancement label Jan 10, 2022

Update test_dispatcher.py

3fc3177

Bibo-Joshi requested a review from Poolitzer January 11, 2022 07:07

Poolitzer requested changes Jan 16, 2022

View reviewed changes

telegram/ext/_dispatcher.py Show resolved Hide resolved

telegram/ext/_dispatcher.py Outdated Show resolved Hide resolved

Merge branch 'v14' into migrate_chat_data + adapt to drop_chat/user_data

d6e49e8

# Conflicts: # telegram/ext/_dispatcher.py

Bibo-Joshi merged commit 8faaa27 into python-telegram-bot:v14 Jan 19, 2022

github-actions bot locked and limited conversation to collaborators Jan 27, 2022

Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024

Add migrate_chat_data method to dispatcher #2848

Add migrate_chat_data method to dispatcher #2848

Uh oh!

Conversation

DonalDuck004 commented Jan 6, 2022

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DonalDuck004 commented Jan 7, 2022

Uh oh!

DonalDuck004 commented Jan 8, 2022

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DonalDuck004 commented Jan 8, 2022

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Poolitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Bibo-Joshi commented Jan 19, 2022

Uh oh!

Uh oh!