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

Jump to bottom

typos #89

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

typos #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91c5b11
typos
KJSanchez Aug 5, 2024
e416b40
PR suggestions
KJSanchez Aug 27, 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
Binary file added assets/dismiss-review.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
7 changes: 5 additions & 2 deletions contributing/responding-pr-review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ all observations but you should respond to all of them with a informative commen
the reviewer knows what you did (addressed the changes, have questions about their
observations, etc.)

It's okay and often faster to use Slack to resolve any disagreement, confusion,
or ambiguity and then update the thread once consensus is reached.

If a code change has been made, include a link to the changes so the reviewer
can find them quickly. First, click on `Files changed` in your PR:

Expand All @@ -30,9 +33,9 @@ Then, your response can be:
[fixed](https://github.com/ploomber/jupysql/pull/787/files#diff-15ef0e119ce73b542976f499fcc3cbb967d30af8199e058aec2bc77c30973061R105-R114)
```

## **Do not** mark the conversations as resolved
## **Do not** mark the conversations as resolved or dismiss reviews

The reviewer is responsible for making conversations as resolved.

![](../assets/resolve-conversation.png)

![](../assets/dismiss-review.png)
4 changes: 2 additions & 2 deletions contributing/submitting-pr.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ If the change breaks the API, the version will be handled case by case. However,

## Backwards compatibility

When breaking the API, we give heads up nnotice to our users so they have enough time to update their code. This involves showing warnings letting them know that a certain feature will be deprecated.
When breaking the API, we give heads up notice to our users so they have enough time to update their code. This involves showing warnings letting them know that a certain feature will be deprecated.

We currently do not have a strict policy so we review cases on a case-by-case basis, but a good rule of thumb is to give at least a month's notice. This implies that Code Owners should ensure that the contributor opens a new PR with deprecation warnings, we merge the PR, and make a new release (by notifying Eduardo or Ido). This process should be prioritized so we make a release as soon as we decide that we'll break the API.

Expand Down Expand Up @@ -392,4 +392,4 @@ Once you finished solving merge conflicts, do a quick check by looking at `Files
Things to look for:

- Are there changes I didn't do but that appear in the `Files changed`? If so, it means that something went wrong when merging conflicts
- Am I missing any changes? If so, you might've deleted some changes while solving merge conflicts
- Am I missing any changes? If so, you might've deleted some changes while solving merge conflicts
Loading