Skip to content

adapt to Python3.8 with PEP570 - #267

Closed
stonebig wants to merge 3 commits into
cloudpipe:masterfrom
stonebig:patch-1
Closed

adapt to Python3.8 with PEP570#267
stonebig wants to merge 3 commits into
cloudpipe:masterfrom
stonebig:patch-1

Conversation

@stonebig

Copy link
Copy Markdown

.... please update also vendored joblib

.... please update also vendored joblib
@codecov

codecovBot commented May 11, 2019

Copy link
Copy Markdown

Codecov Report

Merging #267 into master will decrease coverage by 0.31%.
The diff coverage is 33.33%.

Impacted file tree graph

@@ Coverage Diff @@## master #267 +/- ##
=========================================
- Coverage 88.22% 87.9% -0.32% 
=========================================
Files 1 1 Lines 552 554 +2 Branches 112 113 +1 =========================================
Hits 487 487 - Misses 42 43 +1 - Partials 23 24 +1
Impacted FilesCoverage Δ
cloudpickle/cloudpickle.py87.9% <33.33%> (-0.32%)⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ab5eed...d020f6e. Read the comment docs.

@stonebig

Copy link
Copy Markdown
Author

if you want to be more precise, test should be "if sys.version_info > (3, 8, 0, 'alpha', 3):"

as in https://github.com/ipython/ipython/pull/11720/files#diff-1c766d4a0b1ea9ed8b2d14058b8234ab

@ogriselogrisel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. I am not sure we really need to distinguish alpha releases though. Maybe sys.version_info >= (3, 8) is enough.

Any opinion on this change @pierreglaser?

(),
)

if sys.version_info > (3, 8, 0, 'alpha', 3):

@pierreglaserpierreglaserMay 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe hasattr(types.CodeType, "co_posonlyargcount")?

@pierreglaser

pierreglaser commented May 13, 2019

Copy link
Copy Markdown
Member

This whole part of the code is only useful for versions of python before PEP 3104. For recent python version (python3?), cell_set(cell, value) could just be cell.cell_contents = value instead. +1 For merging this as is once it's ready, and dust this part of the code off later on.

@pierreglaserpierreglaser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test with a function that uses positional only arguments? You also need to change the reducer of code object (Would we have a CI against python 3.8.0.a4, I think every test involving code objects would have failed)

@ogrisel

Copy link
Copy Markdown
Contributor

Closing in favor of #269.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@stonebig@pierreglaser@ogrisel