Do not break loading incompatible cache (#875) #1034
Conversation
OK, good. This will prevent the incompatibility to begin with. |
|
True, but leaving an explicit argument indicates that using the protocol has been considered and choosing the default is deliberate. But if that doesn't sway you, of course I can drop it. 😄 |
|
Would it be better if we include the pickle protocol number in the cache file name? That way it's safe to switch back and forth between different protocols. |
|
After poking through pickle documentation and PEP 3154, my conclusion is that black should pick its own default. The entire purpose of having numbered protocols is to provide for backwards compatibility. Protocol 4 was added in 3.4, and so it is included in all versions that Black supports. Therefore, Black should use |
tests/test_black.py
Outdated
| mode = black.FileMode() | ||
| from shutil import copyfile | ||
|
|
||
| src = THIS_DIR / "data" / "cache-38.pickle" |
There was a problem hiding this comment.
Let me guess this based on file name:
- this is legit py3.8 pickle file
- test is assumed to be ran on py3.7
Would it be better to cook up a pickle file with some crazy protocol number instead?
There was a problem hiding this comment.
Correct. And yes; well; it's currently run on 3.6 and 3.7 during CI checks.
But you're right that an improbable pickle protocol number would be better, since this test could become obsolete. I didn't get at all into pickle internals to set this up (just pickled a tiny black cache in 3.8).
blib2to3/pgen2/token.pyi
Outdated
| @@ -64,6 +64,7 @@ if sys.version_info >= (3, 5): | |||
| AWAIT: int | |||
| ASYNC: int | |||
| ERRORTOKEN: int | |||
| COLONEQUAL: int | |||
There was a problem hiding this comment.
Is this perhaps an unrelated change? If so it could be submitted separately.
There was a problem hiding this comment.
Hrm, that was a mistake. Must have snuck in from my attempt to update the branch to current.
|
Drop the test (it's rather brittle) and the unrelated file change so we can merge this. Thank you for working on this! |
A black cache created in Python 3.8 throws an unhandled
ValueError in earlier versions. This is because 3.6 does
not recognize the pickle protocol used as default in 3.8.
Accordingly, this commit:
- Fixes read_cache to return an empty cache instead.
- Changes the pickle protocol to 4 as the highest protocol
fully supported by black's supported Python versions.
Okay, I've updated to drop the test and properly update my pull request handling -- this should be good to go now. |
|
Thank you, Matt! This makes Black better for everybody. |
|
@ambv thanks!!! is there a release planned with this? 🙇 |
|
No, we'll keep it on master forever. 🤓 Yes, we are planning a release relatively soon now. Please test master at Bloomberg and report any deal breakers before we cut the release this week. |
|
Did a quick rundown of 8-10 projects of the master... Seems to be running mostly just fine. It does generate some changes though, mostly related to either tuple unwrapping (which has now always parenthesis now on the left side) and sometimes remove parenthesis when comments fit in. - expected_txt = (
- "def fun(a1: str, *args: str, kwonly1: int = None, **kwargs) -> None: ...\n"
- ) # noqa
+ expected_txt = "def fun(a1: str, *args: str, kwonly1: int = None, **kwargs) -> None: ...\n" # noqa- msg, = excinfo.value.args
+ (msg,) = excinfo.value.argI get these are expected though. |
|
Argh, it ruined the |
That one looks like #1041. |
A black cache created in Python 3.8 throws a ValueError in earlier versions;
this adds a test for the issue and adjusts read_cache to handle it.