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

Delete entity method and cli #44

Merged
merged 3 commits into from
May 13, 2024
Merged

Delete entity method and cli #44

merged 3 commits into from
May 13, 2024

Conversation

brrttwrks
Copy link
Contributor

I added a delete-entity method for the AlephAPI class and a corresponding cli command to support a needed operation we are facing not infrequenlty lately and also to learn to support alephclient/tools with a seemingly straight-forward example. Let's see how this goes :).

Cases arise where we need to delete specific entities in a dataset.
While the REST API supports it, this will make it a bit easier using
CLI tools and AlephAPI class in code.
Copy link
Contributor

@stchris stchris left a comment

Choose a reason for hiding this comment

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

Generally this looks fine to me, left some minor comments/questions.

alephclient/cli.py Show resolved Hide resolved
@cli.command("delete-entity")
@click.argument("entity_id", required=True)
@click.pass_context
def delete_entity(ctx, entity_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps here as well (if the above works)?

Suggested change
def delete_entity(ctx, entity_id):
def delete_entity(ctx, entity_id: int):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as before - this isn't an integer, but a string that is a hash. How would you recommend I annotate it?

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps as str, that's likely the best we can do here.

res = self.api.write_entity(collection_id, entity)
assert res['id'] == 24
dres = self.api.delete_entity(eid)
assert dres == {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure what our DELETE endpoint returns, but shouldn't we (for completeness sake) try to get the entity with the same ID and expect a 404?

@stchris
Copy link
Contributor

stchris commented Dec 19, 2022

Sorry @brrttwrks the commit signing thing I sent you was only for the last commit, I didn't pay attention and it seems like you had more of them. But perhaps if you squash all your commits after doing the work and then sign that it should work?

@stchris stchris merged commit 65c0f6a into main May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 🏷️ Triage
Development

Successfully merging this pull request may close these issues.

2 participants