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

Make slice deletion consistent with standard lists, fixes #12 #14

Merged
merged 7 commits into from
Aug 4, 2019

Conversation

wetneb
Copy link
Contributor

@wetneb wetneb commented Jul 29, 2019

This reimplements slice deletion to make it consistent with standard lists. Existing test cases have been adapted to reflect this.

See #12.

@johnsyweb johnsyweb self-assigned this Jul 29, 2019
@johnsyweb
Copy link
Owner

Thanks for this PR, @wetneb. The tests look good. I'd like to take some time to review the __delitem__() implementation, as that method is getting quite complex now. I'll try to come back to you this week.

@wetneb
Copy link
Contributor Author

wetneb commented Jul 29, 2019

Yeah, I wish I could have come up with a simpler implementation. Feel free to suggest another version if one comes to mind.

wetneb added a commit to dissemin/dissemin that referenced this pull request Jul 31, 2019
The PR is here: johnsyweb/python_sparse_list#14
Once this is merged and released as sparse_list one may revert this commit.
@johnsyweb
Copy link
Owner

@wetneb How does 3be2d80 look?

@wetneb
Copy link
Contributor Author

wetneb commented Aug 4, 2019

It is definitely more readable, but your implementation has higher asymptotic complexity: O(n m) where n is the number of items in the list and m the number of indices to delete, while mine is O(n + m).

If having complexity O(n m) is not a problem for you I would rewrite your version to use comprehensions as it would probably speed things up a bit.

@johnsyweb
Copy link
Owner

@wetneb Thanks for the analysis. I think 08431bb is better.

sparse_list.py Outdated Show resolved Hide resolved
@wetneb
Copy link
Contributor Author

wetneb commented Aug 4, 2019

Awesome! It is much more readable like that.

@johnsyweb
Copy link
Owner

Great! Thanks for your contribution!

@johnsyweb johnsyweb merged commit f85d96d into johnsyweb:master Aug 4, 2019
@wetneb wetneb deleted the issue-12-slice-deletion branch August 4, 2019 09:16
@wetneb
Copy link
Contributor Author

wetneb commented Aug 4, 2019

@johnsyweb if you get a chance to release this, it would be useful as it would enable django-haystack/django-haystack#1682 to move forward.

@johnsyweb
Copy link
Owner

@wetneb
Copy link
Contributor Author

wetneb commented Aug 4, 2019

@johnsyweb thanks a lot! Interestingly it looks like it is not possible to install it for Python 2.7, probably because only a "wheel" distribution was uploaded to PyPI and not a .tar.gz. Since django-haystack is compatible with Python 2.7 it would be useful to make it available for this version of Python as well.

@johnsyweb
Copy link
Owner

Hmmm...

% python3 -m twine upload dist/*                              

Uploading distributions to https://upload.pypi.org/legacy/
Uploading sparse_list-0.8-py3-none-any.whl
100%|████████████████████████████████████████████████| 11.8k/11.8k [00:03<00:00, 3.33kB/s]
Uploading sparse_list-0.8.tar.gz
100%|████████████████████████████████████████████████| 10.7k/10.7k [00:00<00:00, 27.5kB/s]
NOTE: Try --verbose to see response content.
HTTPError: 403 Client Error: The credential associated with user 'johnsyweb' isn't allowed to upload to project 'sparse-list'. See https://pypi.org/help/#project-name for more information. for url: https://upload.pypi.org/legacy/

Will have another try tonight.

@johnsyweb
Copy link
Owner

Basic auth works (previously I was using an API key) but you'll need pip install sparse-list==0.8.1.

% python3 -m twine upload dist/*         
Uploading distributions to https://upload.pypi.org/legacy/
Uploading sparse_list-0.8.1-py3-none-any.whl
100%|█████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 11.8k/11.8k [00:02<00:00, 5.57kB/s]
Uploading sparse_list-0.8.1.tar.gz
100%|█████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 10.7k/10.7k [00:01<00:00, 7.97kB/s]

@di
Copy link

di commented Aug 4, 2019

@johnsyweb Can you file an issue at https://github.com/pypa/warehouse/issues with as much detail as possible? If you could attach the original 0.8.0 distributions as well that'd be helpful.

@woodruffw
Copy link

Following up here: my current operating theory is that this is caused by API tokens using the non-normalized name, which specifically causes problems for projects with underscores in their names (e.g., sparse_list becomes sparse-list). Normalization isn't reversible, so Project.name for existing uploads becomes the normalized form and our comparison ends up failing.

I'll be confirming this and making the relevant fix(es) in a bit.

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

4 participants