( )⚙ D10770 docket: make compatible with py3.6, where Struct.format is bytes

This is an archive of the discontinued Mercurial Phabricator instance.

docket: make compatible with py3.6, where Struct.format is bytes
AbandonedPublic

Authored by martinvonz on May 25 2021, 7:55 PM.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.May 25 2021, 7:55 PM
marmoute requested changes to this revision.May 28 2021, 9:10 AM
marmoute added a subscriber: marmoute.

Looks like this is happening because of an inconsistency betwen the fact we passe a binary 'struct' for INDEX_HEADER and a sysstr for the extra once. I would rather fix this by making them consistent.

This revision now requires changes to proceed.May 28 2021, 9:10 AM

Looks like this is happening because of an inconsistency betwen the fact we passe a binary 'struct' for INDEX_HEADER and a sysstr for the extra once. I would rather fix this by making them consistent.

I'm not sure I understand. Can you show me how?

$ python3.6 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'bytes'>
$ python3.6 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'bytes'>
$ python3.7 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'str'>
$ python3.7 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'str'>
marmoute added a comment.EditedMay 28 2021, 9:32 AM

Looks like this is happening because of an inconsistency betwen the fact we passe a binary 'struct' for INDEX_HEADER and a sysstr for the extra once. I would rather fix this by making them consistent.

I'm not sure I understand. Can you show me how?

$ python3.6 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'bytes'>
$ python3.6 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'bytes'>
$ python3.7 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'str'>
$ python3.7 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'str'>

Okay, so my assertion isn't correct and Python behavior is… creative. Maybe we should build a pycompat wrapper for struct.format access ?

martinvonz abandoned this revision.May 28 2021, 9:34 AM

Looks like this is happening because of an inconsistency betwen the fact we passe a binary 'struct' for INDEX_HEADER and a sysstr for the extra once. I would rather fix this by making them consistent.

I'm not sure I understand. Can you show me how?

$ python3.6 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'bytes'>
$ python3.6 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'bytes'>
$ python3.7 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'str'>
$ python3.7 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'str'>

Okay, so my assertion is correct and Python behavior is… creative. Maybe we should build a pycompat wrapper for struct.format access ?

Looks like this is happening because of an inconsistency betwen the fact we passe a binary 'struct' for INDEX_HEADER and a sysstr for the extra once. I would rather fix this by making them consistent.

I'm not sure I understand. Can you show me how?

$ python3.6 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'bytes'>
$ python3.6 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'bytes'>
$ python3.7 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'str'>
$ python3.7 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'str'>

Okay, so my assertion is correct and Python behavior is… creative. Maybe we should build a pycompat wrapper for struct.format access ?

I see you are abandoning this revision, can you share details about your new plan ?

Looks like this is happening because of an inconsistency betwen the fact we passe a binary 'struct' for INDEX_HEADER and a sysstr for the extra once. I would rather fix this by making them consistent.

I'm not sure I understand. Can you show me how?

$ python3.6 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'bytes'>
$ python3.6 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'bytes'>
$ python3.7 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'str'>
$ python3.7 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'str'>

Okay, so my assertion is correct and Python behavior is… creative. Maybe we should build a pycompat wrapper for struct.format access ?

AFAICT your assertion was wrong: the struct module just isn't preserving the input type at all, and always emits str on 3.7+, but does bytes on 3.6. So we're not mixing bytes/str input types to struct, but instead struct on 3.6 is ninja-converting back to bytes "for" us.

A pycompat wrapper would make sense for this if we see more than one case of this. Is that likely?

I see you are abandoning this revision, can you share details about your new plan ?

I believe he found the interaction too frustrating and is dealing with other things.

Looks like this is happening because of an inconsistency betwen the fact we passe a binary 'struct' for INDEX_HEADER and a sysstr for the extra once. I would rather fix this by making them consistent.

I'm not sure I understand. Can you show me how?

$ python3.6 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'bytes'>
$ python3.6 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'bytes'>
$ python3.7 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'str'>
$ python3.7 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'str'>

Okay, so my assertion is correct and Python behavior is… creative. Maybe we should build a pycompat wrapper for struct.format access ?

AFAICT your assertion was wrong: the struct module just isn't preserving the input type at all, and always emits str on 3.7+, but does bytes on 3.6. So we're not mixing bytes/str input types to struct, but instead struct on 3.6 is ninja-converting back to bytes "for" us.

Yep, this is a typo on my side (typing is hard) (is right because isn't right)

A pycompat wrapper would make sense for this if we see more than one case of this. Is that likely?

It feels wrong to me to have this kind of compatibility weirdness crawl into normal code. I though more about it and in this case I think the simplest would probably to have the format for INDEX_HEADER in a INDEX_HEADER _FMT constant and concatenate with that.

Looks like this is happening because of an inconsistency betwen the fact we passe a binary 'struct' for INDEX_HEADER and a sysstr for the extra once. I would rather fix this by making them consistent.

I'm not sure I understand. Can you show me how?

$ python3.6 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'bytes'>
$ python3.6 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'bytes'>
$ python3.7 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'str'>
$ python3.7 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'str'>

Okay, so my assertion is correct and Python behavior is… creative. Maybe we should build a pycompat wrapper for struct.format access ?

AFAICT your assertion was wrong: the struct module just isn't preserving the input type at all, and always emits str on 3.7+, but does bytes on 3.6. So we're not mixing bytes/str input types to struct, but instead struct on 3.6 is ninja-converting back to bytes "for" us.

Yep, this is a typo on my side (typing is hard) (is right because isn't right)

A pycompat wrapper would make sense for this if we see more than one case of this. Is that likely?

It feels wrong to me to have this kind of compatibility weirdness crawl into normal code. I though more about it and in this case I think the simplest would probably to have the format for INDEX_HEADER in a INDEX_HEADER _FMT constant and concatenate with that.

Yeah, it feels a bit wrong to me too, but I don't see any great options, esp. w/ the pycompat string tool already doing what we need.

Your solution feels fine-ish, with the caveat that it's more prone to falling out of sync (though I suppose tests would catch that very quickly). I queued this, but if you like the two-constant approach send that as a follow-up?

I will follow the guide here and make docket compatible with py3.6. This is what I want, and I can focus on my australian writings work once this is done. That will make me happy!

Thanks for sharing this article.

Awesome and interesting article. Great things you've always shared with us. Thanks. Just continue composing this kind of post. automatic reply text message