Feature/batch operation #20
Conversation
There was a problem hiding this comment.
Thanks Amirhossein for being ahead of everything.
I left minor comments on your pull request.
Also, I think it might be better to rename Uploader to PypiUploader if the class is scoped only for pypi. Then we can shorten the method's name.
upload_to_pypi->uploadbatch_upload_to_pypi->bacth_upload
It's better to overthink these design choices before the beta version so that we can be backward compatible afterward.
CHANGELOG.md
Outdated
| @@ -6,6 +6,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. | |||
|
|
|||
| ## [Unreleased] | |||
| ### Added | |||
| - Batch of package names reservation | |||
There was a problem hiding this comment.
I prefer something more to-the-point for the user like:
- `batch_upload_to_pypi` method added to `Uploader`
CHANGELOG.md
Outdated
| @@ -6,6 +6,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. | |||
|
|
|||
| ## [Unreleased] | |||
| ### Added | |||
| - Batch of package names reservation | |||
| - testcase for batch package name reservation in `test_reserver.py` | |||
There was a problem hiding this comment.
You can remove this line. Tests are implicitly logged with the main feature.
reserver/reserver_obj.py
Outdated
| for name in names: | ||
| self.upload_to_pypi(name) | ||
|
|
||
|
|
||
| def upload_to_pypi(self, package_name): |
There was a problem hiding this comment.
Why not return True if everything went well?
There was a problem hiding this comment.
well, I should think in more depth about what is False or what is True in this context, but ok I will return sth, maybe the number of names got reserved in this case instead of an ambitious boolean value.
reserver/reserver_obj.py
Outdated
| @@ -29,7 +29,19 @@ def __init__(self, api_token, test_pypi=False): | |||
| self.password = api_token | |||
| self.test_pypi = test_pypi | |||
|
|
|||
|
|
|||
|
|
|||
| def batch_upload_to_pypi(self, *names): | |||
There was a problem hiding this comment.
I prefer the input to be listed. I think that's a more pythonic solution.
Imagine a simple use case from the user's point of view. They should pass their desired package names list by *pacakge_name to the batch_upload_to_pypi which can be a bit weird for novice users.
There was a problem hiding this comment.
with this syntax, the user can do both of these:
.batch_upload_to_pypi(NAME1, NAME2, NAME3) and also .batch_upload_to_pypi([NAME1, NAME2, NAME3])
so you can easily pass the array you want to it.
@sadrasabouri if it's not the case, walk through it more.
There was a problem hiding this comment.
That's not true I guess. If you use the array call version it tries to call the upload method with the list itself instead of elements. The alternative is .batch_upload(*[NAME1, NAME2, NAME3]).
There was a problem hiding this comment.
ok sure I will investigate this, I think we both do have same intension, sure, I will take care of.
There was a problem hiding this comment.
Yes, you are right, thanks
I will take care of it.
@sadrasabouri
There was a problem hiding this comment.
def batch_upload(self, *names):
"""
Upload batch of package names to PyPI.
:param names: packages' names
:type names: vararg
:return: int(number of successfully reserved names)
"""
reserved_successfully = 0
for name in names:
if isinstance(name, list):
reserved_successfully += self.batch_upload(*name)
else:
is_reserved = self.upload(name)
if is_reserved:
reserved_successfully += 1
return reserved_successfullyHappy sir? tests also updated accordingly
@sadrasabouri
reserver/reserver_obj.py
Outdated
| for name in names: | ||
| self.upload_to_pypi(name) | ||
|
|
||
|
|
There was a problem hiding this comment.
If you accept my comments on returning True for upload_to_pypi. This can also return a logical AND of all uploading trials.
There was a problem hiding this comment.
Well, in my opinion, it is way too geeky, I prefer to return the number of actually reserved names here.
tests/test_reserver.py
Outdated
| # test batch of package names | ||
| uploader = Uploader(test_pypi_token, test_pypi= True) | ||
| uploader.batch_upload_to_pypi("numpy", "scikit-learn") | ||
| assert True == True |
There was a problem hiding this comment.
If you accept my comment on returning True for upload_to_pypi, you can assert the output of that function here.
There was a problem hiding this comment.
again with the number of actual names reserved, I can handle associated test cases better and ok sure I will.
@sadrasabouri
| @@ -6,6 +6,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. | |||
|
|
|||
| ## [Unreleased] | |||
| ### Added | |||
| - `batch_upload` method added to `PyPIUploader` | |||
| ### Changed | |||
| - `README.md` modified | |||
There was a problem hiding this comment.
- `Uploader` changed to `PyPIUploader`
There was a problem hiding this comment.
Thanks for you changes.
@sepandhaghighi would you please also take a quick look at the batch_upload implementation and let us know about your thoughts on it?
| def batch_upload(self, *names): | ||
| """ | ||
| Upload batch of package names to PyPI. | ||
|
|
||
| :param names: packages' names | ||
| :type names: vararg | ||
| :return: None |
There was a problem hiding this comment.
Update the doc-string return: None.
|
Hi Sadra, Thank you for your time and review. |
Reference Issues/PRs
#19
What does this implement/fix? Explain your changes.
add ability to batch package name reservation
Any other comments?