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

Start testing PyPy #357

Merged
merged 2 commits into from
Nov 3, 2019
Merged

Start testing PyPy #357

merged 2 commits into from
Nov 3, 2019

Conversation

dargueta
Copy link
Contributor

@dargueta dargueta commented Oct 18, 2019

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

There's a problem with the SFTP server daemon in our tests that causes PyPy to fail on them (see #342). We can still test PyPy compatibility with other parts of the codebase without failing the build by using Travis' allow_failures directive. This will still run tests on PyPy, but won't fail the build if the PyPy builds don't pass. We can remove the allow_failures part once #342 is fixed.

@dargueta
Copy link
Contributor Author

@willmcgugan do you know anything about the MyPy failures?

@willmcgugan
Copy link
Member

No, they appeared recently. It's on my list to take a look at.

I'm traveling atm. Back next week.

@althonos
Copy link
Member

althonos commented Oct 30, 2019

seems like the recent version of mypy wants X.__new__ to return a instance of X, but that's incompatible with the factory pattern we are using in ZipFS and TarFS.

@dargueta
Copy link
Contributor Author

seems like the recent version of mypy

Should we pin it to an earlier version for now?

@althonos
Copy link
Member

Dissecting the version show that the new build fails with mypy == 0.740 but succeeds with mypy == 0.720. Incriminated PR is likely python/mypy#7656.

@willmcgugan
Copy link
Member

I think I'd prefer to ignore those errors rather than pin to older Mypy.

The factory pattern is a little unorthodox, but I wouldn't say it's a bug.

@althonos
Copy link
Member

@willmcgugan : same, this can be ignored IMO.

@lurch
Copy link
Contributor

lurch commented Oct 31, 2019

I know nothing about MyPy, but is it worth reporting a bug / feature-request asking them to support the "unorthodox factory pattern" being used in PyFilesystem? Or is a "local ignore" the only sensible way around this?

@willmcgugan
Copy link
Member

I reckon 99% of the time that would be a bug. So Mypy is right to flag it, but we have a legit reason to ignore those errors.

I'll review that code though. Maybe there is a way of making it less unorthodox that Mypy approves of.

@dargueta
Copy link
Contributor Author

I think I'd prefer to ignore those errors rather than pin to older Mypy.

I forgot we can do that 😄

@willmcgugan
Copy link
Member

Thanks @dargueta

@willmcgugan willmcgugan merged commit d245d72 into PyFilesystem:master Nov 3, 2019
@dargueta dargueta deleted the enable-pypy branch November 5, 2019 22:31
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.

None yet

4 participants