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

Move boto3 logic to library #2

Merged
merged 8 commits into from
Jan 15, 2016
Merged

Conversation

kyleknap
Copy link
Contributor

This transfers all of the boto3 s3 transfer logic to this library. So most of the code has been reviewed already. It also setup the automated testing for this repo through travis and the usual scripts that we use. Here are the things that you may want to look more closely at:

  1. I put the code in a module called legacy to represent that this code eventually will become legacy code as its existence is present because it is used in boto3 and may disappear as the library progresses. I also did this so that I would not have to touch any of that code when I start doing the major refactoring. I also considered putting the code in the __init__.py file, but I did not do that because I do not want that code to be in there for the long run and want to reserve it for higher level stuff like aliasing major classes/functions and having the library version.
  2. The dependencies that I am adding to the library. It currently relies on botocore 1.3.0>=,<2.0.0. It would have been difficult for me to remove it completely so I made sure the dependency range is pretty wide.
  3. Instead of the usual requirements.txt and requirements26.txt, I made one requirements-test.txt as I feel that pip install -r solely should only be used for testing.

Let me know if there is anything else you see.

cc @jamesls @mtdowling @rayluo @JordonPhillips

@jamesls
Copy link
Member

jamesls commented Jan 15, 2016

It seems odd to me to have a brand new package with its main module being legacy. Are you actually planning on changing the interfaces? I would expect we still need to have download_file/upload_file on clients/resources going forward right?

@kyleknap
Copy link
Contributor Author

I am not planning to change the interfaces for backwards compatibility reasons in boto3. However, I do not want users using these interfaces because they are not going to be as featureful as the transfer manager that I plan to add. They are pretty much going to end up being dumbed down versions of the transfer manager with the transfer manager plumbed into the internals. I was hoping to discourage the use of them by placing them in the legacy module.

I just did this a head of time to indicate the intent for this functionality and save some effort. I guess I can rename this module to something else as technically it is not legacy code yet, but this is going to change immediately once the transfer manager is created. What are you thinking? Any suggestions on a different name for the module?

@jamesls
Copy link
Member

jamesls commented Jan 15, 2016

If the back compat is needed for boto3, then I think the legacy adapters should live in boto3, not this package. This package should be written as the "ideal" version, with the preferred package names/interfaces existing here. As long as we get this fixed before we actually push a pypi release, I suppose it doesn't matter what the interim name is. I'd just like to see this corrected fairly soon.

@kyleknap
Copy link
Contributor Author

Yeah ideally these will be wiped out completely. I guess since it is temporary, I will just put it in the logic in __init__.py for now.

@kyleknap
Copy link
Contributor Author

@jamesls I got rid of the legacy module. Good to look at again.

@jamesls
Copy link
Member

jamesls commented Jan 15, 2016

:shipit:

kyleknap added a commit that referenced this pull request Jan 15, 2016
@kyleknap kyleknap merged commit 60b6d7a into boto:develop Jan 15, 2016
@kyleknap kyleknap deleted the move-boto3-logic branch January 15, 2016 22:53
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

2 participants