Skip to content

Conversation

@althonos
Copy link
Member

@althonos althonos commented Jan 5, 2019

PyFilesystem Pull Request

Thank you for your pull request!

Type of changes

  • Bug fix
  • New Feature
  • Documentation / docstrings
  • Other

Checklist

  • I've run the latest black with default args on new code
  • I've updated CHANGELOG.md
  • I accept that @willmcgugan may be pedantic in the code review

Description

I made sure Registry.install returns its argument, otherwise it would not be possible to use it as a decorator as demonstrated in the docstring.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.694% when pulling d678a02 on althonos:master into d6cf403 on PyFilesystem:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.694% when pulling d678a02 on althonos:master into d6cf403 on PyFilesystem:master.

@coveralls
Copy link

coveralls commented Jan 5, 2019

Coverage Status

Coverage remained the same at 99.694% when pulling d678a02 on althonos:master into d6cf403 on PyFilesystem:master.

@willmcgugan
Copy link
Member

Thanks. I'll add a test for this.

@willmcgugan willmcgugan merged commit 18ac2e0 into PyFilesystem:master Jan 6, 2019
@willmcgugan
Copy link
Member

Actually, that wouldn't have broken anything from the registry point of view. The decorator still manages to store a reference to the class. It would have made the opener unimportable as it would have replaced the class with None in the module namespace. Which I guess is why it went undetected!

@althonos
Copy link
Member Author

althonos commented Jan 6, 2019

I noticed that while updating the fs.sshfs opener, and indeed the registry was installed, but in my tests the opener was loaded from the entry point, so the entry_point.load() method would return None, and fs.open_fs('ssh://...') would fail.

@willmcgugan
Copy link
Member

Makes sense. Good spot.

@willmcgugan
Copy link
Member

btw I release 2.2.1 with that fixed.

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.

3 participants