Define SettingCodes and improve ChangedSetting representation. #469
Conversation
Kriechi
force-pushed
the
improve-repr
branch
2 times, most recently
from
66c0e9d
to
6843036
Compare
h2/settings.py
Outdated
| """ | ||
| All known HTTP/2 setting codes. | ||
|
|
||
| .. versionadded:: 2.5.3 |
There was a problem hiding this comment.
This docstring can be dedented.
As to the versionadded comment, this won't be able to make it into the 2.5 release because this is a public part of the API, and so will need to go into 2.6. So the version added is 2.6.0.
h2/settings.py
Outdated
| HEADER_TABLE_SIZE = SettingsFrame.HEADER_TABLE_SIZE | ||
| #: | ||
| #: .. deprecated:: 2.6.0 | ||
| #: Deprecated in favour of :class:`SettingCodes.HEADER_TABLE_SIZE |
There was a problem hiding this comment.
These are all class but should be data, I think.
| MAX_FRAME_SIZE = SettingsFrame.MAX_FRAME_SIZE | ||
| def __init__(self, setting, original_value, new_value): | ||
| #: The setting code given. Either one of :class:`SettingCodes | ||
| #: <h2.settings.SettingCodes>` or ``int`` |
There was a problem hiding this comment.
This should get a .. versionchanged:: 2.6.0 note as well.
h2/settings.py
Outdated
| def __repr__(self): | ||
| return ("ChangedSetting(setting=%s, original_value=%s, " | ||
| "new_value=%s)") % ( | ||
| self.setting.name, |
There was a problem hiding this comment.
Does .name work here in all cases?
There was a problem hiding this comment.
It should... Do you want me to add an if isinstance(self.setting, SettingCodes) check?
There was a problem hiding this comment.
Well, I mean: it definitely won't work for integers, right? 😁
Rather than isinstance, are you happy to just tolerate the default repr of an intenum member?
There was a problem hiding this comment.
How about we make sure that the init of ChangedSetting always converts it to and SettingCodes object?
There was a problem hiding this comment.
You can't convert to an enum if you don't have a member defined for that int.
There was a problem hiding this comment.
Ok, I'll changed it to use the default repr.
Kriechi
force-pushed
the
improve-repr
branch
from
6843036
to
cd6e892
Compare
h2/settings.py
Outdated
| """ | ||
| All known HTTP/2 setting codes. | ||
|
|
||
| .. versionadded:: 2.5.3 |
Kriechi
force-pushed
the
improve-repr
branch
4 times, most recently
from
59be0d2
to
d115923
Compare
h2/events.py
Outdated
| return "<RemoteSettingsChanged changed_settings:%s>" % ( | ||
| self.changed_settings, | ||
| return "<RemoteSettingsChanged changed_settings:{%s}>" % ( | ||
| ", ".join([repr(cs) for cs in self.changed_settings.values()]), |
There was a problem hiding this comment.
Small note here, but you don't need the list. join works just fine on generators, so we can save ourselves the allocation of an intermediate list here and below.
| self.setting, | ||
| self.original_value, | ||
| self.new_value | ||
| ) |
There was a problem hiding this comment.
Wow. This looks weird. It fits in with the code style, so...I guess we'll leave it.
There was a problem hiding this comment.
Either this, or you allow lines with more than 79 characters! 😄
Kriechi
force-pushed
the
improve-repr
branch
from
d115923
to
8620dba
Compare
fixes #359
based on #303