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.

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

Added utils.helpers.effective_message_type #826

Merged
merged 13 commits into from Feb 15, 2018

Conversation

Projects
None yet
3 participants
@JosXa
Copy link
Contributor

JosXa commented Sep 11, 2017

I've added a TODO tag in the helpers file where cyclic imports would happen if I didn't put the import statements inside of the method. This should be fixed before merging.

Added `utils.helpers.effective_message_type` which extracts the type …
…of message as a string from an update or message.

@JosXa JosXa referenced this pull request Sep 11, 2017

Closed

return the type of a message #824

@JosXa JosXa requested a review from Eldinnie Sep 11, 2017

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Sep 14, 2017

I rebased on master. Two things I would like to see on this.
Tests for all occurencies (no type, and an update passed instead of a message)
And please revert the param line in test_message

@Eldinnie
Copy link
Member

Eldinnie left a comment

Tests for all occurencies (no type, and an update passed instead of a message)
And please revert the param line in test_message

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Sep 25, 2017

@JosXa what is the status for this?

@JosXa

This comment has been minimized.

Copy link
Contributor Author

JosXa commented Sep 25, 2017

@Eldinnie I don't have much time atm. Hope to get to all my open issues end of the week

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Sep 25, 2017

@JosXa 👍

@JosXa

This comment has been minimized.

Copy link
Contributor Author

JosXa commented Oct 2, 2017

Made changes as requested.

Question remains: Can/should we do something about the cyclic import (see helpers.py, "TODO")?

@JosXa JosXa self-assigned this Oct 7, 2017

@JosXa JosXa added the enhancement label Oct 7, 2017

@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Feb 1, 2018

@JosXa

This comment has been minimized.

Copy link
Contributor Author

JosXa commented Feb 1, 2018

Integration tests fail because of flood timeouts etc. PR lgtm.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 10, 2018

Codecov Report

Merging #826 into master will decrease coverage by 0.18%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master     #826      +/-   ##
==========================================
- Coverage   91.83%   91.64%   -0.19%     
==========================================
  Files         103      103              
  Lines        4052     4070      +18     
  Branches      639      645       +6     
==========================================
+ Hits         3721     3730       +9     
- Misses        193      198       +5     
- Partials      138      142       +4
Impacted Files Coverage Δ
telegram/message.py 96.26% <100%> (-0.73%) ⬇️
telegram/utils/helpers.py 82.6% <83.33%> (+0.25%) ⬆️
telegram/utils/request.py 67.85% <0%> (-0.9%) ⬇️
telegram/bot.py 87.57% <0%> (-0.44%) ⬇️
@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Feb 10, 2018

@Eldinnie @JosXa
lgtm
i added two small commits.
i think we can merge after CI

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Feb 15, 2018

Coverage is not 100% because this case is never tested:
https://github.com/python-telegram-bot/python-telegram-bot/pull/826/files#diff-f070280279d9dc7a6c79eb51d4ab9156R140
Not a very big problem to me.
Good to merge

@tsnoam

tsnoam approved these changes Feb 15, 2018

@tsnoam tsnoam merged commit 9338dc4 into master Feb 15, 2018

3 of 5 checks passed

codecov/patch 88.23% of diff hit (target 91.83%)
Details
codecov/project 91.64% (-0.19%) compared to 8690ba2
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tsnoam tsnoam deleted the message-type branch Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.