Skip to content

Fix method signatures and expand test_official #2643

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

Merged
merged 17 commits into from
Aug 29, 2021
Merged

Fix method signatures and expand test_official #2643

merged 17 commits into from
Aug 29, 2021

Conversation

harshil21
Copy link
Member

@harshil21 harshil21 commented Aug 26, 2021

Closes #2367

Notes:

  • Didn't remove the args/attributes of ChatMember yet as that's part of Deprecation: v14 #2346. But it's been effectively removed. (Edit: now removed.)
  • Removed __hash__() of VoiceChatParticipantsInvited since its attribute is actually optional. (Edit: added it back)
  • Can someone confirm if I did the de_json of ChatMember* correctly?

Tests:

  • rework test_chatmember to easily test all attributes in one pass
  • test_official now scans if argument if required or not.

@harshil21 harshil21 added bug 🐛 ⚙️ tests affected functionality: tests 🛠 breaking change type: breaking labels Aug 26, 2021
@harshil21 harshil21 added this to the v14 milestone Aug 26, 2021
@harshil21 harshil21 linked an issue Aug 26, 2021 that may be closed by this pull request
didn't commit that weird
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR! Regarding your notes:

  • If you like to, you can remove the ChatMember args & attributes here already. Deprecation: v14 #2346 was split up into mutliple PRs by now anyway. IISC that should also simplify your helper routines for test_chatmember a bit.
  • What exactly is your question about ChatMember.de_json? IISC you only changed the tests, not the actual implementation
  • I left a note about VCPI.__hash__ :)

@harshil21
Copy link
Member Author

  • What exactly is your question about ChatMember.de_json? IISC you only changed the tests, not the actual implementation.

Yes, I didn't change the implementation per se. I just wanted to confirm that ChatMember.de_json(user, 'owner') would no longer work since there's missing arguments. So it had to be changed to ChatMember.de_json(user, 'owner', True). But ig now I've answered my own question

@harshil21 harshil21 mentioned this pull request Aug 27, 2021
22 tasks
@Bibo-Joshi Bibo-Joshi merged commit 48698ea into v14 Aug 29, 2021
@Bibo-Joshi Bibo-Joshi deleted the wrong_meth_sig branch August 29, 2021 16:17
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2021
@Bibo-Joshi Bibo-Joshi added the 🔌 bug pr description: bug label Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛠 breaking change type: breaking 🔌 bug pr description: bug ⚙️ tests affected functionality: tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Wrong signature of API objects/methods
2 participants