Add migrate_chat_data method to dispatcher#2848
Conversation
Bibo-Joshi
left a comment
There was a problem hiding this comment.
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.
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Ok, I'll try tonight/tomorrow |
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
left a comment
There was a problem hiding this comment.
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_datawas 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
left a comment
There was a problem hiding this comment.
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.
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
# Conflicts: # telegram/ext/_dispatcher.py
|
I updated the PR according to #2852 and added an additional test. merging. |
@Bibo-Joshi now there should be no problem, do I have to create a unit_test next?