No empty strings in to_dict by jsmnbom · Pull Request #494 · python-telegram-bot/python-telegram-bot

jsmnbom · 2016-12-30T13:05:34Z

Should remove all cases of having empty strings returned from to_dict (if I didn't miss any).

Closes #460

Should partially fix #460

tsnoam

Ok... I left a review comments on the first classes. But they apply for all of the classes.
Basically, we need to cross-reference with the Telegram Bot API and make sure we don't change their semantics. (I know, a lot of work :-( ) .
That is:

Wherever an attribute is optional - use None as the default.
All attributes must be defined in the class instance (optional attributes value may be None). That goes for int, str, bool...

And of course, unitest must pass...

tsnoam · 2016-12-30T19:29:02Z

telegram/audio.py

+                 performer=None,
+                 title=None,
+                 mime_type=None,
                 file_size=0,


I wonder, should file_size be changed to default to None?

We have that in a few places... I don't really know...
It's the same on with the booleans on Message.

tsnoam · 2016-12-30T19:30:24Z

telegram/audio.py

        self.performer = performer
        self.title = title
-        self.mime_type = str(mime_type)
+        if mime_type:


I think that it will be more accurate to use:
if mime_type is not None
But it's probably ok as it is.

I'mma fix it anyways :)

Actually nvm, I saw your comment further down ^^

tsnoam · 2016-12-30T20:10:21Z

telegram/chat.py

+                 username=None,
+                 first_name=None,
+                 last_name=None,
                 all_members_are_admins=False,


I think all_members_are_admins default should be None. Using False by default changes the telegram semantics.

Are you sure? I think maybe we should just replace all the booleans to None (regardless of what telegrams default is)

tsnoam · 2016-12-31T08:00:36Z

telegram/contact.py

    """

-    def __init__(self, phone_number, first_name, last_name='', user_id=0, **kwargs):
+    def __init__(self, phone_number, first_name, last_name=None, user_id=0, **kwargs):


I think user_id should be default to None, same reasons like with Chat above.

tsnoam · 2016-12-31T08:06:01Z

telegram/document.py

        self.file_name = file_name
-        self.mime_type = str(mime_type)
+        if mime_type:
+            self.mime_type = str(mime_type)


Please make sure self.mime_type is always available. I think that this should be sufficient (no need to use str() here):
self.mime_type = mime_type
If you still think str() is required, you can do:
self.mime_type = str(mime_type) if mime_type is not None else None

This effectively removes most type checking from all optional variables... I'm not really sure that's what we want...

tsnoam

Wow... good job gong through all these methods.
I've pinpointed some minor changes required, also I compared the changes to the API documentation and I found some discrepencies between us, so I believe we should fix them at this chance.
One last thing, please manually merge from or rebase over master because the PR can't be merged automatically.

tsnoam · 2017-01-13T09:23:58Z

telegram/chat.py

+                 username=None,
+                 first_name=None,
+                 last_name=None,
+                 all_members_are_admins=None,


The API of Chat calls this var as all_members_are_administrators, here we use admins. Is there anything that translates admins. I think we actually have a bug here, or telegram might have silently changed the API.

I'm about 90% sure they changed their API... so should we change it, or like keep both, or?

We should follow their official API.
We can do bkwd-compat for a while using **kwargs.
@jh0ker What do you think?

I'd say we keep bkwd-compat

tsnoam · 2017-01-13T09:29:54Z

telegram/file.py

-        self.file_path = str(file_path)
+        self.file_size = file_size
+        if file_path:
+            self.file_path = str(file_path)


Just do
self.file_path = file_path
The if statement should not be there.

Right, whoops. I removed the other ones. Seems like I missed these.

tsnoam · 2017-01-13T09:38:08Z

telegram/user.py

+    def __init__(self,
+                 id,
+                 first_name,
+                 type=None,


I don't see the type attribute in documentation. I think we should remove it too.
I can only guess it's something Telegram had obsoleted.
You should leave it at the prototype until ver. 7 I guess, and put a deprecation warning if it specified.

Hmm.. I don't think it's ever been there tbh.. I might be us who made an error and confused User for Chat or something.

I think Telegram removed it.
@jh0ker what do you think?

I think I remember it from the official docs as well, but I could be wrong...

tsnoam · 2017-01-13T09:42:45Z

telegram/webhookinfo.py

+                 has_custom_certificate,
+                 pending_update_count,
+                 last_error_date=None,
+                 last_error_message=None,


We're missing optionals: max_connections (int) & allowed_updates (List[str])

I noticed but I thought @jh0ker was working on that?
Now that I look at it, it doesn't seem to be in our PR list... (I think I confused it for #483)

I thought that too, so I checked the official December 4th release notes and it wasn't there, so lets fix it here while we're at it.

Wait no... it totally is part of #483 isn't it, @jh0ker?

#494

jsmnbom added 3 commits December 30, 2016 13:57

Fix instances of empty strings in __init__

232a0b0

Should partially fix #460

Fix instances of empty strings in __init__ in example contributing code

651119f

Tricky empty string in __init__

38f2064

tsnoam reviewed Dec 31, 2016

View reviewed changes

jsmnbom added 3 commits January 9, 2017 19:16

Last of the classes I think

4dee785

More IDE friendliness

27b2fd6

Make everything default to None

e69e99c

This effectively removes most type checking from all optional variables... I'm not really sure that's what we want...

tsnoam requested changes Jan 13, 2017

View reviewed changes

jh0ker merged commit e69e99c into master Feb 27, 2017

jh0ker added a commit that referenced this pull request Feb 27, 2017

🔀 Merge master into fix-460

2954ca2

#494

jh0ker added a commit that referenced this pull request Feb 27, 2017

✅ Fix tests

f267646

#494

jh0ker added a commit that referenced this pull request Feb 27, 2017

✅ Fix tests (missed one)

69bfb85

#494

jsmnbom deleted the fix-460 branch May 20, 2017 13:50

github-actions bot locked and limited conversation to collaborators Aug 23, 2020

Conversation

jsmnbom commented Dec 30, 2016

Uh oh!

tsnoam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsnoam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants