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

Fix for Bot.to_dict returning the wrong value for first_name (#1519) #1525

Merged
merged 1 commit into from Oct 11, 2019

Conversation

@clonex10100
Copy link
Contributor

clonex10100 commented Oct 1, 2019

Closes #1519

=

This comment has been minimized.

Copy link
Member

tsnoam commented Oct 1, 2019 — with Telegram GithubBot Revised

@clonex10100 1. We have a very fragile logic with username/first name/last name when it comes to user details.
We should make sure it's either consistent or the difference makes sense.

  1. Unitests seems to fail due to (what I think is) a change in the shell scripting behaviour of Travis. The if... then... logic should be modified to have else true (this is very common behaviour with Makefile)
@Poolitzer

This comment has been minimized.

Copy link
Contributor

Poolitzer commented Oct 10, 2019

Hey @tsnoam , what do you mean?

  1. The bots first name should be the bots first name. Its always given, it should be correctly returned in the to_dict function.

  2. Unitest should not fail because of this change. If there is actually an issue in the file, we should move it to a separate issue and PR, but this one is fine imo

The only issue I see is that the branch isn't named properly and OP didnt add themself to the author file :)

@Poolitzer Poolitzer added this to the 12.2 milestone Oct 10, 2019
@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Oct 11, 2019

@Poolitzer
I would expect test_get_me_and_properties() to fail after this change.
I need to understand why it didn't fail before or why it doesn't fail now.

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Oct 11, 2019

uh. of course. we need a new test for to_dict

This comment has been minimized.

Copy link
Contributor

Poolitzer commented Oct 11, 2019 — with Telegram GithubBot Revised

@tsnoam because that is not affected by to_dict right? They are only using de_json

@tsnoam
tsnoam approved these changes Oct 11, 2019
@tsnoam tsnoam merged commit 2cc9aac into python-telegram-bot:master Oct 11, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.