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.

Sign up for GitHub

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

Feature/batch operation #20

Merged
merged 9 commits into from
Apr 19, 2024
Merged

Feature/batch operation #20

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b4bcaf2
add `batch_upload_to_pypi` to `reserver_obj.py`
AHReccese Apr 14, 2024
6ffef46
add `test_batch_packages_names` testcase
AHReccese Apr 14, 2024
505bf18
`CHANGELOG.md` updated
AHReccese Apr 14, 2024
ccf69f2
apply renamings
AHReccese Apr 15, 2024
3d31199
`CHANGELOG.md` updated
AHReccese Apr 15, 2024
7c5b013
update testcases
AHReccese Apr 15, 2024
ca9c971
enhance `batch_upload` functionality
AHReccese Apr 16, 2024
c916f79
`CHANGELOG.md` updated
AHReccese Apr 16, 2024
ce79109
enhance `batch_upload` testcase
AHReccese Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

I prefer something more to-the-point for the user like:

- `batch_upload_to_pypi` method added to `Uploader`

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

- testcase for batch package name reservation in `test_reserver.py`
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this line. Tests are implicitly logged with the main feature.

### Changed
- `README.md` modified
Copy link
Member

Choose a reason for hiding this comment

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

- `Uploader` changed to `PyPIUploader`

## [0.1] - 2024-02-07
Expand Down
14 changes: 13 additions & 1 deletion reserver/reserver_obj.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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]).

Copy link
Member Author

Choose a reason for hiding this comment

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

ok sure I will investigate this, I think we both do have same intension, sure, I will take care of.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are right, thanks
I will take care of it.
@sadrasabouri

Copy link
Member Author

@AHReccese AHReccese Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

    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_successfully

Happy sir? tests also updated accordingly
@sadrasabouri

"""
Upload batch of package names to PyPI.

:param names: packages' names
:type names: vararg
:return: None
"""
for name in names:
self.upload_to_pypi(name)


Copy link
Member

Choose a reason for hiding this comment

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

If you accept my comments on returning True for upload_to_pypi. This can also return a logical AND of all uploading trials.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, in my opinion, it is way too geeky, I prefer to return the number of actually reserved names here.

def upload_to_pypi(self, package_name):
Copy link
Member

Choose a reason for hiding this comment

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

Why not return True if everything went well?

Copy link
Member Author

@AHReccese AHReccese Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

"""
Upload a template package to pypi or test_pypi.
Expand Down
6 changes: 6 additions & 0 deletions tests/test_reserver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ def test_package_exists():
uploader = Uploader(test_pypi_token, test_pypi= True)
assert uploader.upload_to_pypi("numpy") == False

def test_batch_packages_names():
# test batch of package names
uploader = Uploader(test_pypi_token, test_pypi= True)
uploader.batch_upload_to_pypi("numpy", "scikit-learn")
assert True == True
Copy link
Member

Choose a reason for hiding this comment

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

If you accept my comment on returning True for upload_to_pypi, you can assert the output of that function here.

Copy link
Member Author

Choose a reason for hiding this comment

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

again with the number of actual names reserved, I can handle associated test cases better and ok sure I will.
@sadrasabouri


def test_valid_package_invalid_credentials():
# test not reserved name -> wrong credentials
wrong_pypi_token = "pypi-wrong-api-token"
Expand Down
Loading