docs: Improve contributing docs#12541

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

Merged
mydea merged 4 commits into from
Jun 20, 2024
Merged

docs: Improve contributing docs #12541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f8e1f7b
docs: Improve contributing docs
mydea Jun 19, 2024
1c3a920
small adaptions
mydea Jun 19, 2024
afdb9af
fix prettier
mydea Jun 19, 2024
9b61ec0
Update docs/commit-issue-pr-guidelines.md
mydea Jun 19, 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
82 changes: 13 additions & 69 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -58,11 +58,9 @@ To test local versions of SDK packages, for instance in test projects, you have

**Any nontrivial fixes/features should include tests.** You'll find a `test` folder in each package.

Note that _for the `browser` package only_, if you add a new file to the
[integration test suite](https://github.com/getsentry/sentry-javascript/tree/master/packages/browser/test/integration/suites),
you also need to add it to
[the list in `shell.js`](https://github.com/getsentry/sentry-javascript/blob/b74e199254147fd984e7bb1ea24193aee70afa74/packages/browser/test/integration/suites/shell.js#L25)
as well. Adding tests to existing files will work out of the box in all packages.
For browser related changes, you may also add tests in `dev-packages/browser-integration-tests`. Similarly, for node
integration tests can be added in `dev-packages/node-integration-tests`. Finally, we also have E2E test apps in
`dev-packages/e2e-tests`.

## Running Tests

Expand DownExpand Up@@ -112,79 +110,25 @@ Similar to building and testing, linting can be done in the project root or in i

Note: you must run `yarn build` before `yarn lint` will work.

## Considerations Before Sending Your First PR
## External Contributors

When contributing to the codebase, please note:

- Make sure to follow the [Commit, Issue & PR guidelines](#commit-issue--pr-guidelines)
- Non-trivial PRs will not be accepted without tests (see above).
- Please do not bump version numbers yourself.
- [`raven-js`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-js) and
[`raven-node`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-node) are deprecated, and only
bug and security fix PRs will be accepted targeting the
[3.x branch](https://github.com/getsentry/sentry-javascript/tree/3.x). Any new features and improvements should be to
our new SDKs (`browser`, `node`, and framework-specific packages like `react` and `nextjs`) and the packages which
support them (`core`, `utils`, `integrations`, and the like).

## PR reviews

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.
We highly appreciate external contributions to the SDK. If you want to contribute something, you can just open a PR
against `develop`.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate.
The SDK team will check out your PR shortly!

Our different types of reviews:
When contributing to the codebase, please note:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
`h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
this sparingly.
- Make sure to follow the [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md)
- Non-trivial PRs will not be accepted without tests (see above).

## Commit, Issue & PR guidelines

### Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

### Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

### Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto master.

- The PR name can generally follow the commit name (e.g.
`feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `master` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
number

### Gitflow
See [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md).

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.
## PR Reviews

For more details, [see our Gitflow docs](./docs/gitflow.md).
See [PR Reviews](./docs/pr-reviews.md).

## Publishing a Release

Expand Down
40 changes: 40 additions & 0 deletions docs/commit-issue-pr-guidelines.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
# Commit, Issue & PR guidelines

## Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

## Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

## Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto `develop`.

- The PR name can generally follow the commit name (e.g.
`feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `develop` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
number

Please note that we cannot _enforce_ Squash Merge due to the usage of Gitflow (see below). Github remembers the last
used merge method, so you'll need to make sure to double check that you are using "Squash and Merge" correctly.

## Gitflow

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.

For more details, [see our Gitflow docs](./gitflow.md).
42 changes: 42 additions & 0 deletions docs/pr-reviews.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
# PR reviews

Make sure to open PRs against `develop` branch.

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate. You can either assign SDK team members directly (e.g. if you have some people in mind who are
well suited to review a PR), or you can assign `getsentry/team-web-sdk-frontend`, which will randomly pick 2 people from
the team to assign.

Our different types of reviews:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
`h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
this sparingly.

You show generally avoid to use "Auto merge". The reason is that we have some CI workflows which do not block merging
(e.g. flaky test detection, some optional E2E tests). If these fail, and you enabled Auto Merge, the PR will be merged
if though some workflow(s) failed. To avoid this, wait for CI to pass to merge the PR manually, or only enable "Auto
Merge" if you know that no optional workflow may fail. Another reason is that, as stated above in 2., reviewers may
leave comments and directly approve the PR. In this case, as PR author you should review the comments and choose which
to implement and which may be ignored for now. "Auto Merge" leads to the PR feedback not being taken into account.

## Reviewing a PR from an external contributor

1. Make sure to review PRs from external contributors in a timely fashion. These users spent their valuable time to
improve our SDK, so we should not leave them hanging with a review!
2. Make sure to click "Approve and Run" on the CI for the PR, if it does not seem malicious.
3. Provide feedback and guidance if the PR is not ready to be merged.
4. Assign the PR to yourself if you start reviewing it. You are then responsible for guiding the PR either to
completion, or to close it if it does not align with the goals of the SDK team.
5. Make sure to update the PR name to align with our commit name structure (see above)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
docs: Improve contributing docs#12541

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

 Merged

mydea
merged 4 commits into

from

Jun 20, 2024
 Merged


docs: Improve contributing docs
#12541
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
f8e1f7b

docs: Improve contributing docs


mydea Jun 19, 2024
1c3a920

small adaptions


mydea Jun 19, 2024
afdb9af

fix prettier


mydea Jun 19, 2024
9b61ec0

Update docs/commit-issue-pr-guidelines.md


mydea Jun 19, 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


82 changes: 13 additions & 69 deletions
CONTRIBUTING.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,11 +58,9 @@ To test local versions of SDK packages, for instance in test projects, you have

**Any nontrivial fixes/features should include tests.** You'll find a `test` folder in each package.

Note that _for the `browser` package only_, if you add a new file to the
[integration test suite](https://github.com/getsentry/sentry-javascript/tree/master/packages/browser/test/integration/suites),
you also need to add it to
[the list in `shell.js`](https://github.com/getsentry/sentry-javascript/blob/b74e199254147fd984e7bb1ea24193aee70afa74/packages/browser/test/integration/suites/shell.js#L25)
as well. Adding tests to existing files will work out of the box in all packages.
For browser related changes, you may also add tests in `dev-packages/browser-integration-tests`. Similarly, for node
integration tests can be added in `dev-packages/node-integration-tests`. Finally, we also have E2E test apps in
`dev-packages/e2e-tests`.

## Running Tests

Expand DownExpand Up@@ -112,79 +110,25 @@ Similar to building and testing, linting can be done in the project root or in i

Note: you must run `yarn build` before `yarn lint` will work.

## Considerations Before Sending Your First PR
## External Contributors

When contributing to the codebase, please note:

- Make sure to follow the [Commit, Issue & PR guidelines](#commit-issue--pr-guidelines)
- Non-trivial PRs will not be accepted without tests (see above).
- Please do not bump version numbers yourself.
- [`raven-js`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-js) and
 [`raven-node`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-node) are deprecated, and only
 bug and security fix PRs will be accepted targeting the
 [3.x branch](https://github.com/getsentry/sentry-javascript/tree/3.x). Any new features and improvements should be to
 our new SDKs (`browser`, `node`, and framework-specific packages like `react` and `nextjs`) and the packages which
 support them (`core`, `utils`, `integrations`, and the like).

## PR reviews

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.
We highly appreciate external contributions to the SDK. If you want to contribute something, you can just open a PR
against `develop`.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate.
The SDK team will check out your PR shortly!

Our different types of reviews:
When contributing to the codebase, please note:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.
- Make sure to follow the [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md)
- Non-trivial PRs will not be accepted without tests (see above).

## Commit, Issue & PR guidelines

### Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

### Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

### Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto master.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `master` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

### Gitflow
See [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md).

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.
## PR Reviews

For more details, [see our Gitflow docs](./docs/gitflow.md).
See [PR Reviews](./docs/pr-reviews.md).

## Publishing a Release

Expand Down

40 changes: 40 additions & 0 deletions
docs/commit-issue-pr-guidelines.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
# Commit, Issue & PR guidelines

## Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

## Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

## Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto `develop`.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `develop` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

Please note that we cannot _enforce_ Squash Merge due to the usage of Gitflow (see below). Github remembers the last
used merge method, so you'll need to make sure to double check that you are using "Squash and Merge" correctly.

## Gitflow

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.

For more details, [see our Gitflow docs](./gitflow.md).

42 changes: 42 additions & 0 deletions
docs/pr-reviews.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
# PR reviews

Make sure to open PRs against `develop` branch.

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate. You can either assign SDK team members directly (e.g. if you have some people in mind who are
well suited to review a PR), or you can assign `getsentry/team-web-sdk-frontend`, which will randomly pick 2 people from
the team to assign.

Our different types of reviews:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.

You show generally avoid to use "Auto merge". The reason is that we have some CI workflows which do not block merging
(e.g. flaky test detection, some optional E2E tests). If these fail, and you enabled Auto Merge, the PR will be merged
if though some workflow(s) failed. To avoid this, wait for CI to pass to merge the PR manually, or only enable "Auto
Merge" if you know that no optional workflow may fail. Another reason is that, as stated above in 2., reviewers may
leave comments and directly approve the PR. In this case, as PR author you should review the comments and choose which
to implement and which may be ignored for now. "Auto Merge" leads to the PR feedback not being taken into account.

## Reviewing a PR from an external contributor

1. Make sure to review PRs from external contributors in a timely fashion. These users spent their valuable time to
 improve our SDK, so we should not leave them hanging with a review!
2. Make sure to click "Approve and Run" on the CI for the PR, if it does not seem malicious.
3. Provide feedback and guidance if the PR is not ready to be merged.
4. Assign the PR to yourself if you start reviewing it. You are then responsible for guiding the PR either to
 completion, or to close it if it does not align with the goals of the SDK team.
5. Make sure to update the PR name to align with our commit name structure (see above)

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
docs: Improve contributing docs#12541

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

 Merged

mydea
merged 4 commits into

from

Jun 20, 2024
 Merged


docs: Improve contributing docs
#12541
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
f8e1f7b

docs: Improve contributing docs


mydea Jun 19, 2024
1c3a920

small adaptions


mydea Jun 19, 2024
afdb9af

fix prettier


mydea Jun 19, 2024
9b61ec0

Update docs/commit-issue-pr-guidelines.md


mydea Jun 19, 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


82 changes: 13 additions & 69 deletions
CONTRIBUTING.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,11 +58,9 @@ To test local versions of SDK packages, for instance in test projects, you have

**Any nontrivial fixes/features should include tests.** You'll find a `test` folder in each package.

Note that _for the `browser` package only_, if you add a new file to the
[integration test suite](https://github.com/getsentry/sentry-javascript/tree/master/packages/browser/test/integration/suites),
you also need to add it to
[the list in `shell.js`](https://github.com/getsentry/sentry-javascript/blob/b74e199254147fd984e7bb1ea24193aee70afa74/packages/browser/test/integration/suites/shell.js#L25)
as well. Adding tests to existing files will work out of the box in all packages.
For browser related changes, you may also add tests in `dev-packages/browser-integration-tests`. Similarly, for node
integration tests can be added in `dev-packages/node-integration-tests`. Finally, we also have E2E test apps in
`dev-packages/e2e-tests`.

## Running Tests

Expand DownExpand Up@@ -112,79 +110,25 @@ Similar to building and testing, linting can be done in the project root or in i

Note: you must run `yarn build` before `yarn lint` will work.

## Considerations Before Sending Your First PR
## External Contributors

When contributing to the codebase, please note:

- Make sure to follow the [Commit, Issue & PR guidelines](#commit-issue--pr-guidelines)
- Non-trivial PRs will not be accepted without tests (see above).
- Please do not bump version numbers yourself.
- [`raven-js`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-js) and
 [`raven-node`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-node) are deprecated, and only
 bug and security fix PRs will be accepted targeting the
 [3.x branch](https://github.com/getsentry/sentry-javascript/tree/3.x). Any new features and improvements should be to
 our new SDKs (`browser`, `node`, and framework-specific packages like `react` and `nextjs`) and the packages which
 support them (`core`, `utils`, `integrations`, and the like).

## PR reviews

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.
We highly appreciate external contributions to the SDK. If you want to contribute something, you can just open a PR
against `develop`.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate.
The SDK team will check out your PR shortly!

Our different types of reviews:
When contributing to the codebase, please note:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.
- Make sure to follow the [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md)
- Non-trivial PRs will not be accepted without tests (see above).

## Commit, Issue & PR guidelines

### Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

### Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

### Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto master.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `master` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

### Gitflow
See [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md).

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.
## PR Reviews

For more details, [see our Gitflow docs](./docs/gitflow.md).
See [PR Reviews](./docs/pr-reviews.md).

## Publishing a Release

Expand Down

40 changes: 40 additions & 0 deletions
docs/commit-issue-pr-guidelines.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
# Commit, Issue & PR guidelines

## Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

## Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

## Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto `develop`.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `develop` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

Please note that we cannot _enforce_ Squash Merge due to the usage of Gitflow (see below). Github remembers the last
used merge method, so you'll need to make sure to double check that you are using "Squash and Merge" correctly.

## Gitflow

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.

For more details, [see our Gitflow docs](./gitflow.md).

42 changes: 42 additions & 0 deletions
docs/pr-reviews.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
# PR reviews

Make sure to open PRs against `develop` branch.

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate. You can either assign SDK team members directly (e.g. if you have some people in mind who are
well suited to review a PR), or you can assign `getsentry/team-web-sdk-frontend`, which will randomly pick 2 people from
the team to assign.

Our different types of reviews:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.

You show generally avoid to use "Auto merge". The reason is that we have some CI workflows which do not block merging
(e.g. flaky test detection, some optional E2E tests). If these fail, and you enabled Auto Merge, the PR will be merged
if though some workflow(s) failed. To avoid this, wait for CI to pass to merge the PR manually, or only enable "Auto
Merge" if you know that no optional workflow may fail. Another reason is that, as stated above in 2., reviewers may
leave comments and directly approve the PR. In this case, as PR author you should review the comments and choose which
to implement and which may be ignored for now. "Auto Merge" leads to the PR feedback not being taken into account.

## Reviewing a PR from an external contributor

1. Make sure to review PRs from external contributors in a timely fashion. These users spent their valuable time to
 improve our SDK, so we should not leave them hanging with a review!
2. Make sure to click "Approve and Run" on the CI for the PR, if it does not seem malicious.
3. Provide feedback and guidance if the PR is not ready to be merged.
4. Assign the PR to yourself if you start reviewing it. You are then responsible for guiding the PR either to
 completion, or to close it if it does not align with the goals of the SDK team.
5. Make sure to update the PR name to align with our commit name structure (see above)

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
docs: Improve contributing docs#12541

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

 Merged

mydea
merged 4 commits into

from

Jun 20, 2024
 Merged


docs: Improve contributing docs
#12541
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
f8e1f7b

docs: Improve contributing docs


mydea Jun 19, 2024
1c3a920

small adaptions


mydea Jun 19, 2024
afdb9af

fix prettier


mydea Jun 19, 2024
9b61ec0

Update docs/commit-issue-pr-guidelines.md


mydea Jun 19, 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


82 changes: 13 additions & 69 deletions
CONTRIBUTING.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,11 +58,9 @@ To test local versions of SDK packages, for instance in test projects, you have

**Any nontrivial fixes/features should include tests.** You'll find a `test` folder in each package.

Note that _for the `browser` package only_, if you add a new file to the
[integration test suite](https://github.com/getsentry/sentry-javascript/tree/master/packages/browser/test/integration/suites),
you also need to add it to
[the list in `shell.js`](https://github.com/getsentry/sentry-javascript/blob/b74e199254147fd984e7bb1ea24193aee70afa74/packages/browser/test/integration/suites/shell.js#L25)
as well. Adding tests to existing files will work out of the box in all packages.
For browser related changes, you may also add tests in `dev-packages/browser-integration-tests`. Similarly, for node
integration tests can be added in `dev-packages/node-integration-tests`. Finally, we also have E2E test apps in
`dev-packages/e2e-tests`.

## Running Tests

Expand DownExpand Up@@ -112,79 +110,25 @@ Similar to building and testing, linting can be done in the project root or in i

Note: you must run `yarn build` before `yarn lint` will work.

## Considerations Before Sending Your First PR
## External Contributors

When contributing to the codebase, please note:

- Make sure to follow the [Commit, Issue & PR guidelines](#commit-issue--pr-guidelines)
- Non-trivial PRs will not be accepted without tests (see above).
- Please do not bump version numbers yourself.
- [`raven-js`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-js) and
 [`raven-node`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-node) are deprecated, and only
 bug and security fix PRs will be accepted targeting the
 [3.x branch](https://github.com/getsentry/sentry-javascript/tree/3.x). Any new features and improvements should be to
 our new SDKs (`browser`, `node`, and framework-specific packages like `react` and `nextjs`) and the packages which
 support them (`core`, `utils`, `integrations`, and the like).

## PR reviews

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.
We highly appreciate external contributions to the SDK. If you want to contribute something, you can just open a PR
against `develop`.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate.
The SDK team will check out your PR shortly!

Our different types of reviews:
When contributing to the codebase, please note:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.
- Make sure to follow the [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md)
- Non-trivial PRs will not be accepted without tests (see above).

## Commit, Issue & PR guidelines

### Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

### Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

### Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto master.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `master` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

### Gitflow
See [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md).

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.
## PR Reviews

For more details, [see our Gitflow docs](./docs/gitflow.md).
See [PR Reviews](./docs/pr-reviews.md).

## Publishing a Release

Expand Down

40 changes: 40 additions & 0 deletions
docs/commit-issue-pr-guidelines.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
# Commit, Issue & PR guidelines

## Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

## Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

## Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto `develop`.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `develop` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

Please note that we cannot _enforce_ Squash Merge due to the usage of Gitflow (see below). Github remembers the last
used merge method, so you'll need to make sure to double check that you are using "Squash and Merge" correctly.

## Gitflow

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.

For more details, [see our Gitflow docs](./gitflow.md).

42 changes: 42 additions & 0 deletions
docs/pr-reviews.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
# PR reviews

Make sure to open PRs against `develop` branch.

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate. You can either assign SDK team members directly (e.g. if you have some people in mind who are
well suited to review a PR), or you can assign `getsentry/team-web-sdk-frontend`, which will randomly pick 2 people from
the team to assign.

Our different types of reviews:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.

You show generally avoid to use "Auto merge". The reason is that we have some CI workflows which do not block merging
(e.g. flaky test detection, some optional E2E tests). If these fail, and you enabled Auto Merge, the PR will be merged
if though some workflow(s) failed. To avoid this, wait for CI to pass to merge the PR manually, or only enable "Auto
Merge" if you know that no optional workflow may fail. Another reason is that, as stated above in 2., reviewers may
leave comments and directly approve the PR. In this case, as PR author you should review the comments and choose which
to implement and which may be ignored for now. "Auto Merge" leads to the PR feedback not being taken into account.

## Reviewing a PR from an external contributor

1. Make sure to review PRs from external contributors in a timely fashion. These users spent their valuable time to
 improve our SDK, so we should not leave them hanging with a review!
2. Make sure to click "Approve and Run" on the CI for the PR, if it does not seem malicious.
3. Provide feedback and guidance if the PR is not ready to be merged.
4. Assign the PR to yourself if you start reviewing it. You are then responsible for guiding the PR either to
 completion, or to close it if it does not align with the goals of the SDK team.
5. Make sure to update the PR name to align with our commit name structure (see above)

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
docs: Improve contributing docs#12541

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

 Merged

mydea
merged 4 commits into

from

Jun 20, 2024
 Merged


docs: Improve contributing docs
#12541
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
f8e1f7b

docs: Improve contributing docs


mydea Jun 19, 2024
1c3a920

small adaptions


mydea Jun 19, 2024
afdb9af

fix prettier


mydea Jun 19, 2024
9b61ec0

Update docs/commit-issue-pr-guidelines.md


mydea Jun 19, 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


82 changes: 13 additions & 69 deletions
CONTRIBUTING.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,11 +58,9 @@ To test local versions of SDK packages, for instance in test projects, you have

**Any nontrivial fixes/features should include tests.** You'll find a `test` folder in each package.

Note that _for the `browser` package only_, if you add a new file to the
[integration test suite](https://github.com/getsentry/sentry-javascript/tree/master/packages/browser/test/integration/suites),
you also need to add it to
[the list in `shell.js`](https://github.com/getsentry/sentry-javascript/blob/b74e199254147fd984e7bb1ea24193aee70afa74/packages/browser/test/integration/suites/shell.js#L25)
as well. Adding tests to existing files will work out of the box in all packages.
For browser related changes, you may also add tests in `dev-packages/browser-integration-tests`. Similarly, for node
integration tests can be added in `dev-packages/node-integration-tests`. Finally, we also have E2E test apps in
`dev-packages/e2e-tests`.

## Running Tests

Expand DownExpand Up@@ -112,79 +110,25 @@ Similar to building and testing, linting can be done in the project root or in i

Note: you must run `yarn build` before `yarn lint` will work.

## Considerations Before Sending Your First PR
## External Contributors

When contributing to the codebase, please note:

- Make sure to follow the [Commit, Issue & PR guidelines](#commit-issue--pr-guidelines)
- Non-trivial PRs will not be accepted without tests (see above).
- Please do not bump version numbers yourself.
- [`raven-js`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-js) and
 [`raven-node`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-node) are deprecated, and only
 bug and security fix PRs will be accepted targeting the
 [3.x branch](https://github.com/getsentry/sentry-javascript/tree/3.x). Any new features and improvements should be to
 our new SDKs (`browser`, `node`, and framework-specific packages like `react` and `nextjs`) and the packages which
 support them (`core`, `utils`, `integrations`, and the like).

## PR reviews

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.
We highly appreciate external contributions to the SDK. If you want to contribute something, you can just open a PR
against `develop`.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate.
The SDK team will check out your PR shortly!

Our different types of reviews:
When contributing to the codebase, please note:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.
- Make sure to follow the [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md)
- Non-trivial PRs will not be accepted without tests (see above).

## Commit, Issue & PR guidelines

### Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

### Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

### Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto master.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `master` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

### Gitflow
See [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md).

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.
## PR Reviews

For more details, [see our Gitflow docs](./docs/gitflow.md).
See [PR Reviews](./docs/pr-reviews.md).

## Publishing a Release

Expand Down

40 changes: 40 additions & 0 deletions
docs/commit-issue-pr-guidelines.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
# Commit, Issue & PR guidelines

## Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

## Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

## Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto `develop`.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `develop` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

Please note that we cannot _enforce_ Squash Merge due to the usage of Gitflow (see below). Github remembers the last
used merge method, so you'll need to make sure to double check that you are using "Squash and Merge" correctly.

## Gitflow

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.

For more details, [see our Gitflow docs](./gitflow.md).

42 changes: 42 additions & 0 deletions
docs/pr-reviews.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
# PR reviews

Make sure to open PRs against `develop` branch.

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate. You can either assign SDK team members directly (e.g. if you have some people in mind who are
well suited to review a PR), or you can assign `getsentry/team-web-sdk-frontend`, which will randomly pick 2 people from
the team to assign.

Our different types of reviews:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.

You show generally avoid to use "Auto merge". The reason is that we have some CI workflows which do not block merging
(e.g. flaky test detection, some optional E2E tests). If these fail, and you enabled Auto Merge, the PR will be merged
if though some workflow(s) failed. To avoid this, wait for CI to pass to merge the PR manually, or only enable "Auto
Merge" if you know that no optional workflow may fail. Another reason is that, as stated above in 2., reviewers may
leave comments and directly approve the PR. In this case, as PR author you should review the comments and choose which
to implement and which may be ignored for now. "Auto Merge" leads to the PR feedback not being taken into account.

## Reviewing a PR from an external contributor

1. Make sure to review PRs from external contributors in a timely fashion. These users spent their valuable time to
 improve our SDK, so we should not leave them hanging with a review!
2. Make sure to click "Approve and Run" on the CI for the PR, if it does not seem malicious.
3. Provide feedback and guidance if the PR is not ready to be merged.
4. Assign the PR to yourself if you start reviewing it. You are then responsible for guiding the PR either to
 completion, or to close it if it does not align with the goals of the SDK team.
5. Make sure to update the PR name to align with our commit name structure (see above)

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
docs: Improve contributing docs#12541

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

 Merged

mydea
merged 4 commits into

from

Jun 20, 2024
 Merged


docs: Improve contributing docs
#12541
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
f8e1f7b

docs: Improve contributing docs


mydea Jun 19, 2024
1c3a920

small adaptions


mydea Jun 19, 2024
afdb9af

fix prettier


mydea Jun 19, 2024
9b61ec0

Update docs/commit-issue-pr-guidelines.md


mydea Jun 19, 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


82 changes: 13 additions & 69 deletions
CONTRIBUTING.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,11 +58,9 @@ To test local versions of SDK packages, for instance in test projects, you have

**Any nontrivial fixes/features should include tests.** You'll find a `test` folder in each package.

Note that _for the `browser` package only_, if you add a new file to the
[integration test suite](https://github.com/getsentry/sentry-javascript/tree/master/packages/browser/test/integration/suites),
you also need to add it to
[the list in `shell.js`](https://github.com/getsentry/sentry-javascript/blob/b74e199254147fd984e7bb1ea24193aee70afa74/packages/browser/test/integration/suites/shell.js#L25)
as well. Adding tests to existing files will work out of the box in all packages.
For browser related changes, you may also add tests in `dev-packages/browser-integration-tests`. Similarly, for node
integration tests can be added in `dev-packages/node-integration-tests`. Finally, we also have E2E test apps in
`dev-packages/e2e-tests`.

## Running Tests

Expand DownExpand Up@@ -112,79 +110,25 @@ Similar to building and testing, linting can be done in the project root or in i

Note: you must run `yarn build` before `yarn lint` will work.

## Considerations Before Sending Your First PR
## External Contributors

When contributing to the codebase, please note:

- Make sure to follow the [Commit, Issue & PR guidelines](#commit-issue--pr-guidelines)
- Non-trivial PRs will not be accepted without tests (see above).
- Please do not bump version numbers yourself.
- [`raven-js`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-js) and
 [`raven-node`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-node) are deprecated, and only
 bug and security fix PRs will be accepted targeting the
 [3.x branch](https://github.com/getsentry/sentry-javascript/tree/3.x). Any new features and improvements should be to
 our new SDKs (`browser`, `node`, and framework-specific packages like `react` and `nextjs`) and the packages which
 support them (`core`, `utils`, `integrations`, and the like).

## PR reviews

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.
We highly appreciate external contributions to the SDK. If you want to contribute something, you can just open a PR
against `develop`.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate.
The SDK team will check out your PR shortly!

Our different types of reviews:
When contributing to the codebase, please note:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.
- Make sure to follow the [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md)
- Non-trivial PRs will not be accepted without tests (see above).

## Commit, Issue & PR guidelines

### Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

### Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

### Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto master.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `master` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

### Gitflow
See [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md).

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.
## PR Reviews

For more details, [see our Gitflow docs](./docs/gitflow.md).
See [PR Reviews](./docs/pr-reviews.md).

## Publishing a Release

Expand Down

40 changes: 40 additions & 0 deletions
docs/commit-issue-pr-guidelines.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
# Commit, Issue & PR guidelines

## Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

## Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

## Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto `develop`.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `develop` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

Please note that we cannot _enforce_ Squash Merge due to the usage of Gitflow (see below). Github remembers the last
used merge method, so you'll need to make sure to double check that you are using "Squash and Merge" correctly.

## Gitflow

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.

For more details, [see our Gitflow docs](./gitflow.md).

42 changes: 42 additions & 0 deletions
docs/pr-reviews.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
# PR reviews

Make sure to open PRs against `develop` branch.

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate. You can either assign SDK team members directly (e.g. if you have some people in mind who are
well suited to review a PR), or you can assign `getsentry/team-web-sdk-frontend`, which will randomly pick 2 people from
the team to assign.

Our different types of reviews:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.

You show generally avoid to use "Auto merge". The reason is that we have some CI workflows which do not block merging
(e.g. flaky test detection, some optional E2E tests). If these fail, and you enabled Auto Merge, the PR will be merged
if though some workflow(s) failed. To avoid this, wait for CI to pass to merge the PR manually, or only enable "Auto
Merge" if you know that no optional workflow may fail. Another reason is that, as stated above in 2., reviewers may
leave comments and directly approve the PR. In this case, as PR author you should review the comments and choose which
to implement and which may be ignored for now. "Auto Merge" leads to the PR feedback not being taken into account.

## Reviewing a PR from an external contributor

1. Make sure to review PRs from external contributors in a timely fashion. These users spent their valuable time to
 improve our SDK, so we should not leave them hanging with a review!
2. Make sure to click "Approve and Run" on the CI for the PR, if it does not seem malicious.
3. Provide feedback and guidance if the PR is not ready to be merged.
4. Assign the PR to yourself if you start reviewing it. You are then responsible for guiding the PR either to
 completion, or to close it if it does not align with the goals of the SDK team.
5. Make sure to update the PR name to align with our commit name structure (see above)

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
docs: Improve contributing docs#12541

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

 Merged

mydea
merged 4 commits into

from

Jun 20, 2024
 Merged


docs: Improve contributing docs
#12541
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
f8e1f7b

docs: Improve contributing docs


mydea Jun 19, 2024
1c3a920

small adaptions


mydea Jun 19, 2024
afdb9af

fix prettier


mydea Jun 19, 2024
9b61ec0

Update docs/commit-issue-pr-guidelines.md


mydea Jun 19, 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


82 changes: 13 additions & 69 deletions
CONTRIBUTING.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,11 +58,9 @@ To test local versions of SDK packages, for instance in test projects, you have

**Any nontrivial fixes/features should include tests.** You'll find a `test` folder in each package.

Note that _for the `browser` package only_, if you add a new file to the
[integration test suite](https://github.com/getsentry/sentry-javascript/tree/master/packages/browser/test/integration/suites),
you also need to add it to
[the list in `shell.js`](https://github.com/getsentry/sentry-javascript/blob/b74e199254147fd984e7bb1ea24193aee70afa74/packages/browser/test/integration/suites/shell.js#L25)
as well. Adding tests to existing files will work out of the box in all packages.
For browser related changes, you may also add tests in `dev-packages/browser-integration-tests`. Similarly, for node
integration tests can be added in `dev-packages/node-integration-tests`. Finally, we also have E2E test apps in
`dev-packages/e2e-tests`.

## Running Tests

Expand DownExpand Up@@ -112,79 +110,25 @@ Similar to building and testing, linting can be done in the project root or in i

Note: you must run `yarn build` before `yarn lint` will work.

## Considerations Before Sending Your First PR
## External Contributors

When contributing to the codebase, please note:

- Make sure to follow the [Commit, Issue & PR guidelines](#commit-issue--pr-guidelines)
- Non-trivial PRs will not be accepted without tests (see above).
- Please do not bump version numbers yourself.
- [`raven-js`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-js) and
 [`raven-node`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-node) are deprecated, and only
 bug and security fix PRs will be accepted targeting the
 [3.x branch](https://github.com/getsentry/sentry-javascript/tree/3.x). Any new features and improvements should be to
 our new SDKs (`browser`, `node`, and framework-specific packages like `react` and `nextjs`) and the packages which
 support them (`core`, `utils`, `integrations`, and the like).

## PR reviews

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.
We highly appreciate external contributions to the SDK. If you want to contribute something, you can just open a PR
against `develop`.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate.
The SDK team will check out your PR shortly!

Our different types of reviews:
When contributing to the codebase, please note:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.
- Make sure to follow the [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md)
- Non-trivial PRs will not be accepted without tests (see above).

## Commit, Issue & PR guidelines

### Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

### Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

### Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto master.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `master` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

### Gitflow
See [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md).

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.
## PR Reviews

For more details, [see our Gitflow docs](./docs/gitflow.md).
See [PR Reviews](./docs/pr-reviews.md).

## Publishing a Release

Expand Down

40 changes: 40 additions & 0 deletions
docs/commit-issue-pr-guidelines.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
# Commit, Issue & PR guidelines

## Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

## Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

## Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto `develop`.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `develop` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

Please note that we cannot _enforce_ Squash Merge due to the usage of Gitflow (see below). Github remembers the last
used merge method, so you'll need to make sure to double check that you are using "Squash and Merge" correctly.

## Gitflow

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.

For more details, [see our Gitflow docs](./gitflow.md).

42 changes: 42 additions & 0 deletions
docs/pr-reviews.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
# PR reviews

Make sure to open PRs against `develop` branch.

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate. You can either assign SDK team members directly (e.g. if you have some people in mind who are
well suited to review a PR), or you can assign `getsentry/team-web-sdk-frontend`, which will randomly pick 2 people from
the team to assign.

Our different types of reviews:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.

You show generally avoid to use "Auto merge". The reason is that we have some CI workflows which do not block merging
(e.g. flaky test detection, some optional E2E tests). If these fail, and you enabled Auto Merge, the PR will be merged
if though some workflow(s) failed. To avoid this, wait for CI to pass to merge the PR manually, or only enable "Auto
Merge" if you know that no optional workflow may fail. Another reason is that, as stated above in 2., reviewers may
leave comments and directly approve the PR. In this case, as PR author you should review the comments and choose which
to implement and which may be ignored for now. "Auto Merge" leads to the PR feedback not being taken into account.

## Reviewing a PR from an external contributor

1. Make sure to review PRs from external contributors in a timely fashion. These users spent their valuable time to
 improve our SDK, so we should not leave them hanging with a review!
2. Make sure to click "Approve and Run" on the CI for the PR, if it does not seem malicious.
3. Provide feedback and guidance if the PR is not ready to be merged.
4. Assign the PR to yourself if you start reviewing it. You are then responsible for guiding the PR either to
 completion, or to close it if it does not align with the goals of the SDK team.
5. Make sure to update the PR name to align with our commit name structure (see above)

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
docs: Improve contributing docs#12541

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

 Merged

mydea
merged 4 commits into

from

Jun 20, 2024
 Merged


docs: Improve contributing docs
#12541
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


4 commits


Select commit
Hold shift + click to select a range
f8e1f7b

docs: Improve contributing docs


mydea Jun 19, 2024
1c3a920

small adaptions


mydea Jun 19, 2024
afdb9af

fix prettier


mydea Jun 19, 2024
9b61ec0

Update docs/commit-issue-pr-guidelines.md


mydea Jun 19, 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


82 changes: 13 additions & 69 deletions
CONTRIBUTING.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,11 +58,9 @@ To test local versions of SDK packages, for instance in test projects, you have

**Any nontrivial fixes/features should include tests.** You'll find a `test` folder in each package.

Note that _for the `browser` package only_, if you add a new file to the
[integration test suite](https://github.com/getsentry/sentry-javascript/tree/master/packages/browser/test/integration/suites),
you also need to add it to
[the list in `shell.js`](https://github.com/getsentry/sentry-javascript/blob/b74e199254147fd984e7bb1ea24193aee70afa74/packages/browser/test/integration/suites/shell.js#L25)
as well. Adding tests to existing files will work out of the box in all packages.
For browser related changes, you may also add tests in `dev-packages/browser-integration-tests`. Similarly, for node
integration tests can be added in `dev-packages/node-integration-tests`. Finally, we also have E2E test apps in
`dev-packages/e2e-tests`.

## Running Tests

Expand DownExpand Up@@ -112,79 +110,25 @@ Similar to building and testing, linting can be done in the project root or in i

Note: you must run `yarn build` before `yarn lint` will work.

## Considerations Before Sending Your First PR
## External Contributors

When contributing to the codebase, please note:

- Make sure to follow the [Commit, Issue & PR guidelines](#commit-issue--pr-guidelines)
- Non-trivial PRs will not be accepted without tests (see above).
- Please do not bump version numbers yourself.
- [`raven-js`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-js) and
 [`raven-node`](https://github.com/getsentry/sentry-javascript/tree/3.x/packages/raven-node) are deprecated, and only
 bug and security fix PRs will be accepted targeting the
 [3.x branch](https://github.com/getsentry/sentry-javascript/tree/3.x). Any new features and improvements should be to
 our new SDKs (`browser`, `node`, and framework-specific packages like `react` and `nextjs`) and the packages which
 support them (`core`, `utils`, `integrations`, and the like).

## PR reviews

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.
We highly appreciate external contributions to the SDK. If you want to contribute something, you can just open a PR
against `develop`.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate.
The SDK team will check out your PR shortly!

Our different types of reviews:
When contributing to the codebase, please note:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.
- Make sure to follow the [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md)
- Non-trivial PRs will not be accepted without tests (see above).

## Commit, Issue & PR guidelines

### Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

### Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

### Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto master.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `master` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

### Gitflow
See [Commit, Issue & PR guidelines](./docs/commit-issue-pr-guidelines.md).

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.
## PR Reviews

For more details, [see our Gitflow docs](./docs/gitflow.md).
See [PR Reviews](./docs/pr-reviews.md).

## Publishing a Release

Expand Down

40 changes: 40 additions & 0 deletions
docs/commit-issue-pr-guidelines.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
# Commit, Issue & PR guidelines

## Commits

For commit messages, we use the format:

```
<type>(<scope>): <subject> (<github-id>)
```

For example: `feat(core): Set custom transaction source for event processors (#5722)`.

See [commit message format](https://develop.sentry.dev/commit-messages/#commit-message-format) for details.

The Github-ID can be left out until the PR is merged.

## Issues

Issues should at least be categorized by package, for example `package: Node`. Additional labels for categorization can
be added, and the Sentry SDK team may also add further labels as needed.

## Pull Requests (PRs)

PRs are merged via `Squash and merge`. This means that all commits on the branch will be squashed into a single commit,
and committed as such onto `develop`.

- The PR name can generally follow the commit name (e.g.
 `feat(core): Set custom transaction source for event processors`)
- Make sure to rebase the branch on `develop` before squashing it
- Make sure to update the commit message of the squashed branch to follow the commit guidelines - including the PR
 number

Please note that we cannot _enforce_ Squash Merge due to the usage of Gitflow (see below). Github remembers the last
used merge method, so you'll need to make sure to double check that you are using "Squash and Merge" correctly.

## Gitflow

We use [Gitflow](https://docs.github.com/en/get-started/quickstart/github-flow) as a branching model.

For more details, [see our Gitflow docs](./gitflow.md).

42 changes: 42 additions & 0 deletions
docs/pr-reviews.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
# PR reviews

Make sure to open PRs against `develop` branch.

For feedback in PRs, we use the [LOGAF scale](https://blog.danlew.net/2020/04/15/the-logaf-scale/) to specify how
important a comment is:

- `l`: low - nitpick. You may address this comment, but you don't have to.
- `m`: medium - normal comment. Worth addressing and fixing.
- `h`: high - Very important. We must not merge this PR without addressing this issue.

You only need one approval from a maintainer to be able to merge. For some PRs, asking specific or multiple people for
review might be adequate. You can either assign SDK team members directly (e.g. if you have some people in mind who are
well suited to review a PR), or you can assign `getsentry/team-web-sdk-frontend`, which will randomly pick 2 people from
the team to assign.

Our different types of reviews:

1. **LGTM without any comments.** You can merge immediately.
2. **LGTM with low and medium comments.** The reviewer trusts you to resolve these comments yourself, and you don't need
 to wait for another approval.
3. **Only comments.** You must address all the comments and need another review until you merge.
4. **Request changes.** Only use if something critical is in the PR that absolutely must be addressed. We usually use
 `h` comments for that. When someone requests changes, the same person must approve the changes to allow merging. Use
 this sparingly.

You show generally avoid to use "Auto merge". The reason is that we have some CI workflows which do not block merging
(e.g. flaky test detection, some optional E2E tests). If these fail, and you enabled Auto Merge, the PR will be merged
if though some workflow(s) failed. To avoid this, wait for CI to pass to merge the PR manually, or only enable "Auto
Merge" if you know that no optional workflow may fail. Another reason is that, as stated above in 2., reviewers may
leave comments and directly approve the PR. In this case, as PR author you should review the comments and choose which
to implement and which may be ignored for now. "Auto Merge" leads to the PR feedback not being taken into account.

## Reviewing a PR from an external contributor

1. Make sure to review PRs from external contributors in a timely fashion. These users spent their valuable time to
 improve our SDK, so we should not leave them hanging with a review!
2. Make sure to click "Approve and Run" on the CI for the PR, if it does not seem malicious.
3. Provide feedback and guidance if the PR is not ready to be merged.
4. Assign the PR to yourself if you start reviewing it. You are then responsible for guiding the PR either to
 completion, or to close it if it does not align with the goals of the SDK team.
5. Make sure to update the PR name to align with our commit name structure (see above)