-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Jupyter notebook tutorial - Delete API #14781
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested the code yet, but this is a great start.
Along with the suggestions, these are some things to watch out for:
- Avoid using pronouns like we/our/us. Generally, you should just be directing the user, so
you
most of the time - Avoid using future tense (
will
) where possible
For a v1 notebook, I think it's fine thatdruidapi
isn't used. That can be tackled in a subsequent PR.
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions and nits
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
"id": "a4e8453e", | ||
"metadata": {}, | ||
"source": [ | ||
"In addition to deleting by interval, you can delete segments by using `segment_id`. Let's load in some new data to work with.\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this comes after we delete the whole table? it seems inefficient to have them have to reingest a table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I grouped deleting by interval and table together since they both use the same base method, passing a time interval.
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, the changes look great. I tested the calls, and they worked.
Just a few nits but nothing that should block merging this.
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
examples/quickstart/jupyter-notebooks/notebooks/04-api/01-delete-api-tutorial.ipynb
Outdated
Show resolved
Hide resolved
This pull request has been marked as stale due to 60 days of inactivity. |
This pull request/issue has been closed due to lack of activity. If you think that |
This PR:
This PR has: