- Notifications
You must be signed in to change notification settings - Fork5.7k
User like properties#4713
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
User like properties#4713
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…as in the `telegram.User` class.
…sts for it; Replace `user` and `shared_user` correspond properties implementations on a new functinos.
…rnames.py` implementation; Note: `full_name` and `effective_name` can not be easily replaced in the such way
…lved; Slots tests failed for Protocol class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the updates! I had just a very quick look so far
- I'm not sure if I have used overloads correctly, as warnings are still present.
I don't see anything wrong at first glance, but haven't seen the mypy warnings (pre-commit truncated the output). Looking up other code, have a look at
python-telegram-bot/telegram/_utils/argumentparsing.py
Lines 71 to 94 in1503287
| Tele_co=TypeVar("Tele_co",bound=TelegramObject,covariant=True) | |
| TeleCrypto_co=TypeVar("TeleCrypto_co",bound="HasDecryptMethod",covariant=True) | |
| ifTYPE_CHECKING: | |
| @type_check_only | |
| classHasDecryptMethod(Protocol): | |
| __slots__= () | |
| @classmethod | |
| defde_json_decrypted( | |
| cls:type[TeleCrypto_co], | |
| data:JSONDict, | |
| bot:Optional["Bot"], | |
| credentials:list["FileCredentials"], | |
| )->TeleCrypto_co: ... | |
| @classmethod | |
| defde_list_decrypted( | |
| cls:type[TeleCrypto_co], | |
| data:list[JSONDict], | |
| bot:Optional["Bot"], | |
| credentials:list["FileCredentials"], | |
| )->tuple[TeleCrypto_co, ...]: ... |
for how Protocol and typevar are used there. maybe that helps.
- There is a failed slots test for the class.
in the above code section theif TYPE_CHECKING ensures that the class is not defined at test runtime. this would get the test passing.
- Is it necessary to test the properties of the User class, or utils.usernames is sufficient to test ?
I'd say testing the properties of the User(/Chat/SharedUser) class is enough and explicitly testing_utils.usernames is not required :)
Please also install thepre-commit hooks
| # You should have received a copy of the GNU Lesser Public License | ||
| # along with this program. If not, see [http://www.gnu.org/licenses/]. | ||
| """Shared properties to extract username, first_name, last_name values if filled.""" | ||
| from __future__ import annotations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please avoid using this. If this is for using|, please useUnion andOptional instead
| unlike the `Chat` and `Shared`, were they are optional. | ||
| The `last_name` is always optional. | ||
| """ | ||
| last_name: str | None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| last_name:str|None | |
| first_name:Optional[str] | |
| last_name:str|None |
that way you should be able to removeMiniUserLike
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- One of the hooks, probably black or isort, converts automatically during commit.
- Sometimes, the field always exists (for example, for the
Usertype) and sometimes it might not (like in theSharedUsertype). This affects the return type: it can beOptional[str]or guaranteed to be anstr. That's why I introduced it. - I know the name
MiniUserLikeis pretty silly, but I couldn't think of anything better. I hope you can help me with that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- running the pre-commit hooks on python 3.9 (lowest supported version) should make sure that this doesn't happen. Though I admit I'm not completely sure why it does in the first place
- you already differentiate between
UserLikeOptionalandUserLike. That should already do the trick IMO
@david-shiko are you still interested in finishing this up? |
Hi, yes, I'm interested, but I don't have any free time and I don't know when it will be, you can close it, and if anything, I'll make a new PR or open this one. |
Uh oh!
There was an error while loading.Please reload this page.
This is a basic implementation of the changes suggested in thepull request 4708 .
Please note that there are several issues with the code:
Userclass, orutils.usernamesis sufficient to test ?