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

Fix missing ENOTSUP on PyPy #339

Merged
merged 7 commits into from Aug 30, 2019
Merged

Conversation

dargueta
Copy link
Contributor

@dargueta dargueta commented Aug 6, 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

  1. Only add errno.ENOTSUP to FTP failure codes if that's present.
  2. Removed a few unused imports.

Closes #338

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0002%) to 99.001% when pulling 554ce16 on dargueta:pypy-enotsup into 84719be on PyFilesystem:master.

@coveralls
Copy link

coveralls commented Aug 6, 2019

Coverage Status

Coverage remained the same at 99.001% when pulling a7c3a96 on dargueta:pypy-enotsup into 84719be on PyFilesystem:master.

@lurch
Copy link
Contributor

lurch commented Aug 6, 2019

I wonder if it might be better to fix this with try ... catch rather than adding arbitrary error numbers? 🤷‍♂️

@lurch
Copy link
Contributor

lurch commented Aug 6, 2019

P.S. Is this at all relevant? giampaolo/pysendfile#28

@dargueta
Copy link
Contributor Author

dargueta commented Aug 6, 2019

I wonder if it might be better to fix this with try ... catch rather than adding arbitrary error numbers?

And what would we do on the catch?

Never mind, I see. We could omit that error code from _sendfile_error_codes if it's not present. Probably a better idea.

P.S. Is this at all relevant? giampaolo/pysendfile#28

I don't think so?

@dargueta
Copy link
Contributor Author

dargueta commented Aug 6, 2019

Crashes in PyPy appear to be unrelated, due to an SFTP connection issue. Stack trace here.

fs/osfs.py Show resolved Hide resolved
@dargueta dargueta mentioned this pull request Aug 6, 2019
9 tasks
fs/osfs.py Outdated Show resolved Hide resolved
@willmcgugan
Copy link
Member

The pypy build is failing on the ftpfs tests. I've seen this before and never figured it out. It's unrelated to your changes.

I'm happy to merge this and treat the pypy test fails as another task. What do you think?

@dargueta
Copy link
Contributor Author

dargueta commented Aug 8, 2019

I'm happy to merge this and treat the pypy test fails as another task.

Conceptually it makes sense, but anyone creating a new PR is probably gonna be really mad that their PR is broken because of something I did. Up to you.

@willmcgugan
Copy link
Member

How about remove the pypy builds from travis for now, and I'll add an issue to add them later?

@willmcgugan
Copy link
Member

Added an issue. #342

If you wouldn't mind removing the pypy builds I'll merge this...

@dargueta
Copy link
Contributor Author

dargueta commented Aug 8, 2019

This is gonna have a merge conflict with the pytest PR #337 which modifies the Travis file and will remove the PyPy tests. Let's get that in first and then this will be good to go.

@willmcgugan
Copy link
Member

@dargueta Would you mind fixing that merge conflict and we should be good to go.

for info in self._read_dir(_path).values():
yield info
for info in self._read_dir(_path).values():
yield info
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change have any impact on non-pypy usages of pyfilesystem?

Copy link
Member

Choose a reason for hiding this comment

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

I think this was purely because the lock is redundant, as there is a lock on L714.

Copy link
Contributor Author

@dargueta dargueta Aug 27, 2019

Choose a reason for hiding this comment

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

I think this was purely because the lock is redundant, as there is a lock on L714.

Yep. I thought this was the cause of a "cannot release lock not held" (or whatever that message was) that only shows up on PyPy. Apparently this isn't, because that warning still shows up.

@@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Fixed incorrect imports of `mock` on Python 3
- Removed some unused imports and unused `requirements.txt` file
- Added mypy checks to Travis
- Fixed missing `errno.ENOTSUP` on PyPy. Closes [#338](https://github.com/PyFilesystem/pyfilesystem2/issues/338).
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this PR also close #342 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point. Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay never mind, I give up. Let's not do 342 in this PR.

@dargueta
Copy link
Contributor Author

@willmcgugan I think we're good now.

@willmcgugan
Copy link
Member

Thanks @dargueta !

@willmcgugan willmcgugan merged commit 2d9189d into PyFilesystem:master Aug 30, 2019
@dargueta dargueta deleted the pypy-enotsup branch August 30, 2019 15:13
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.

Bug: PyPy does not have ENOTSUP
4 participants