Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for python 3.8 #3719

Merged
merged 4 commits into from
Jan 20, 2020
Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jan 20, 2020

Fixes #3662

Note, I have not checked the compatibility of all our (sub)dependencies manually, but the tests run.

This behavior will raise starting from python 3.8.
This behaviour is no longer supported in python 3.8:

    python/cpython@3715176557cf

For an enum `SomeEnum`, one can no longer do:

    if 'SOME_VALUE' not in SomeEnum:
	raise ValueError()

but instead one should use `isinstance` directly. After all an enum
is simply a normal python class.
@sphuber sphuber force-pushed the fix_3662_python_3_8 branch from c0d14f1 to b985838 Compare January 20, 2020 10:28
ltalirz
ltalirz previously approved these changes Jan 20, 2020
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @sphuber , a smooth python version transition for once ?!

@sphuber
Copy link
Contributor Author

sphuber commented Jan 20, 2020

thanks @sphuber , a smooth python version transition for once ?!

Cheered to early! Tests on my branch run, but on the PR they fail in one of the file system tests. There seems to be a problem with shutil handling utf-8 encoded strings. Not sure why yet

dev-zero
dev-zero previously approved these changes Jan 20, 2020
Copy link
Contributor

@dev-zero dev-zero left a comment

Choose a reason for hiding this comment

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

LGTM, interesting that those issues have so far gone unnoticed

The test was calling `insert_path` from a filepath into itself. When
moving to python 3.8 this was triggering a weird recursive condition in
the underlying `shutil.copytree` call that kept concatenating the
`šaltinis/destination` sub string to the target filepath. This would
eventually throw an error that the filepath was too long. The origin for
this occurring all of a sudden in python 3.8 is unsure, but most likely
the recursive copy in the test was a typo and not intended.
Run CI tests on python 3.8 instead of 3.7.
@sphuber
Copy link
Contributor Author

sphuber commented Jan 20, 2020

@dev-zero and @ltalirz thanks for the review. As you may have noticed, the tests on my branch ran, but on the pull request failed. Specifically it was a single test testing the Folder utility when handling utf-8 encoded file paths. I could not reproduce it locally either and so not figure out what was going wrong. I added a commit which gets rid of the problem. I think what was being tested was not the original intention of the author. The test now makes sense. Of course this is not wholly satisfactory because the original test should be valid and run without exceptions, however, I have no clue where to start looking nor the time to do it. For now I think this is an OK solution then, but please give it a look first.

@sphuber sphuber requested review from dev-zero and ltalirz January 20, 2020 15:13
@dev-zero
Copy link
Contributor

@sphuber I think that's fine since the test is about unicode handling of path names rather than the logic of Folder.insert_path (and shutils behaviour).

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Yeah - while it's possible that python 3.8 shutil handles paths differently, it does seem strange strange that the issue did not appear locally for you...
Anyhow, let's move on ;-)

@sphuber sphuber merged commit 381d941 into aiidateam:develop Jan 20, 2020
@sphuber sphuber deleted the fix_3662_python_3_8 branch January 20, 2020 18:35
Sign up for free to 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.

Python 3.8 compatibility issues
3 participants