Conversation
Bug: T206334 Change-Id: I5c0122f7c1765a41fa172e2a073def7b54720f79
|
I tested on all ways possible and it worked just fine. |
adamwight
left a comment
adamwight
left a comment
There was a problem hiding this comment.
I'm fine with merging as-is, but had one cleanup question.
| "ip={0!r}".format(self.ip), | ||
| "model_info={0!r}".format(self.model_info)])) | ||
|
|
||
| def toJSON(self): |
There was a problem hiding this comment.
I don't think "JSON" is the right word for what we're doing here. Technically, it's "marshalling" although that doesn't exactly roll off the tongue. Also, Python breaks the term by misusing it as another name for serialization.
What do you think about "toData" and "newFromData"?
There was a problem hiding this comment.
Can you say more why you don't think "JSON" is the right term? Maybe you mean "JSONable" since it's not turning the data structure into JSON, but rather a trivially JSON-able python data structure.
There was a problem hiding this comment.
Oh also the name is a style issue. Should probably be "to_json" or something like that.
There was a problem hiding this comment.
Renamed them to to_json and from_json
There was a problem hiding this comment.
I renamed it to to_json
There was a problem hiding this comment.
Exactly, I was recommending against to_json because it doesn't create JSON. It's technically https://en.wikipedia.org/wiki/Marshalling_(computer_science) which is not serialization, we could pickle this data structure and so on.
This isn't a blocker though.
| 'model_info': self.model_info | ||
| } | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
Nice use of classmethod :-)
There was a problem hiding this comment.
Got it from pywikibot 🙈🙈
| kwargs['context'], | ||
| kwargs['rev_ids'], | ||
| kwargs['model_names'], | ||
| precache=kwargs.get('precache', False), |
There was a problem hiding this comment.
Curious why there are defaults here. It doesn't seem like the fields are optional in our serialized format, at least.
There was a problem hiding this comment.
Indeed. Why not just do cls(**kwargs)?
Also, I think this is a confusing use of "kwargs" since it generally refers to key word arguments, but here, you are taking a deserialized JSON document.
| } | ||
|
|
||
| @classmethod | ||
| def fromJSON(cls, kwargs): |
Change-Id: I9db4baefab36df6cc170432246b5a613b4dd38ab
3 participants
Bug: T206334