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

WIP: feature/api improvements #41

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

flavioamieiro
Copy link
Member

This Pull Request is still a work in progress, so it should probably not be merged yet. I'm publishing it here so I can get reviews.

I've added methods to delete the objects (instead of having to call the delete method from requests with the entity URL). The biggest change, though, is returning Document objects in the when accessing the documents attribute of a Corpus. This change breaks backward compatibility, so we need to be sure we want that before we merge this (we also need to bump our major version before we publish to pypi).

I'd like you all @rsouza , @fccoelho and @eduardostalinho to review this, please. Specially the test_get_all_documents test.

This method allows a user to delete the corpus using the corpus object itself,
instead of needing to use `requests.delete` and the corpus url.
This method allows a user to delete a document using the document object
itself, instead of needing to use `requests.delete` and the document url.
This *breaks backwards compatibility*. This means we probably need to bump
the major version before we merge this into master and publish to pypi.
@rsouza
Copy link
Member

rsouza commented Jun 9, 2016

I don't know what is the expected behavior when unsucessfully trying to delet, but i would raise an informative error - maybe like the index error when trying to pop() from an empty list?

@@ -140,6 +156,9 @@ def __init__(self, session, *args, **kwargs):
'''
self.session = session
for key, value in kwargs.items():
if key == 'documents':
value = map(lambda d: Document(session=self.session, **d),
Copy link
Member

Choose a reason for hiding this comment

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

I like this, because its way more usefull for API users. No need to worry about breaking compatibility since we don't have many users yet. But we should make a clear note about this in some form of release note.

@flavioamieiro
Copy link
Member Author

@rsouza the way the code is right now it will raise a RuntimeError if there is any problem while trying to delete.

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.

3 participants