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

Allowing boto keys to be passed directly to smart_open (#35) #38

Merged
merged 4 commits into from Dec 18, 2015

Conversation

asieira
Copy link
Contributor

@asieira asieira commented Nov 5, 2015

  • Updated smart_open to accept an instance of boto.s3.key.Key as the
    uri parameter.
  • Refactored S3OpenRead and S3OpenWrite to require only a
    boto.s3.key.Key instance to work, and get the bucket directly from its
    properties. This is not backwards compatible.
  • Commented out test SmartOpenReadTest.test_s3_boto temporarily.

* Updated `smart_open` to accept an instance of boto.s3.key.Key as the
`uri` parameter.
* Refactored `S3OpenRead` and `S3OpenWrite` to require only a
boto.s3.key.Key instance to work, and get the bucket directly from its
properties. *This is not backwards compatible*.
* Commented out test `SmartOpenReadTest.test_s3_boto` temporarily.
@asieira
Copy link
Contributor Author

asieira commented Nov 5, 2015

I commented out one of the tests since I'm not very familiar with moto and I'm probably doing something wrong. This is the output I get:

Error
Traceback (most recent call last):
  File "/Users/asieira/.virtualenvs/smart_open/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "/Users/asieira/Repositorios/smart_open/smart_open/tests/test_smart_open.py", line 139, in test_s3_boto
    smart_open_object = smart_open.smart_open("s3://mybucket/mykey")
  File "/Users/asieira/Repositorios/smart_open/smart_open/smart_open_lib.py", line 111, in smart_open
    return S3OpenRead(key, **kw)
  File "/Users/asieira/Repositorios/smart_open/smart_open/smart_open_lib.py", line 222, in __init__
    if not isinstance(read_key, boto.s3.key.Key):
TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types

@asieira asieira mentioned this pull request Nov 5, 2015
@piskvorky
Copy link
Owner

Thanks @asieira ! The PR looks great.

That's a weird error, not sure what's going on. Will have to check in more detail.

@asieira
Copy link
Contributor Author

asieira commented Nov 5, 2015

🙇

smart_open_object.__iter__()
mock_boto.connect_s3().get_bucket.assert_called_with("mybucket")
mock_boto.connect_s3().get_bucket().lookup.assert_called_with("mykey")
self.assertTrue(mock_s3_iter_lines.called)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please uncomment the test as we need to merge them in.

@tmylk
Copy link
Contributor

tmylk commented Dec 16, 2015

@asieira

See comments in code. Refer to #45 for examples.

Please add the change description to the CHANGELOG and then it will be ready to merge

@tmylk
Copy link
Contributor

tmylk commented Dec 16, 2015

@asieira Thanks for the quick response. Waiting for the commit - going to release today.

Implemented @tmylk ’s recommendations:
* Fixed `test_s3_boto` by replacing usage of `lookup` with `get_key`
and re-enabled it;
* Fixed type checking in the constructors of `HdfsOpenRead` and
`HdfsOpenWrite`;
* Updated doctoring to reflect new usage of `smart_open`;
* Updated CHANGELOG to reflect this change.
@asieira
Copy link
Contributor Author

asieira commented Dec 16, 2015

There you go, @tmylk !

tmylk added a commit that referenced this pull request Dec 18, 2015
Allowing boto keys to be passed directly to smart_open (#35)
@tmylk tmylk merged commit 23f6921 into piskvorky:master Dec 18, 2015
@asieira
Copy link
Contributor Author

asieira commented Dec 18, 2015

I see the new version was released as 1.3.1.

I would suggest that in the future the project follow the semantic versioning guidelines, which IMHO is what most developers expect these days. Under these guidelines, since new functionality was introduced by my PR, I believe the release version would have been 1.4.0.

No crisis, just a suggestion for future releases. :) Thanks again for the wonderful project!

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

3 participants