Skip to content
22 changes: 2 additions & 20 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -1,26 +1,8 @@
<ins>**Please make sure your changes are properly tested**!</ins>
Comment thread
watson marked this conversation as resolved.

### What does this PR do?
<!-- A brief description of the change being made with this pull request. -->

### Motivation
<!-- What inspired you to submit this pull request? -->

### Plugin Checklist
<!-- Fill this section if adding or updating a plugin. Remove otherwise. -->

- [ ] Unit tests.
- [ ] Integration tests.
- [ ] Benchmarks.
- [ ] TypeScript [definitions][1].
- [ ] TypeScript [tests][2].
- [ ] API [documentation][3].
- [ ] CI [jobs/workflows][4].

[1]: https://github.com/DataDog/dd-trace-js/blob/master/index.d.ts
[2]: https://github.com/DataDog/dd-trace-js/blob/master/docs/test.ts
[3]: https://github.com/DataDog/documentation/blob/master/content/en/tracing/trace_collection/library_config/nodejs.md
[4]: https://github.com/DataDog/dd-trace-js/blob/master/.github/workflows/plugins.yml

### Additional Notes
<!-- Anything else we should know when reviewing? -->
Comment thread
simon-id marked this conversation as resolved.


15 changes: 15 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -214,4 +214,19 @@ $ yarn bench
```


### Checklist
Comment thread
simon-id marked this conversation as resolved.
Outdated
Here is a simple checklist to help write a more complete PR:
- [ ] Unit tests.
- [ ] Integration tests.
- [ ] Benchmarks.
- [ ] TypeScript [definitions][2].
- [ ] TypeScript [tests][3].
- [ ] API [documentation][4].
- [ ] CI [jobs/workflows][5].

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why the [ ]? This is no longer an issue/PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so people that want to use the checklist can copy paste it in their description or somewhere else ;)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If it's meant to be copied it should be a code block, otherwise it's not possible. In its current state, it just shows disabled checkboxes that can't be checked

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's a compromise between making it look good in markdown, and making the original copyable. Either you're reading the markdown in an editor and the original copyable is there, or you're on github interface and there is literally a button to switch to the original:
image

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I question the usefulness of this and would probably vote to just remove this section altogether

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with removing it. It was a request from @bengl on my original slack thread.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think he intended for people to click "edit source" to actually use it 😉

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i'll have to let him say what he intended himself, don't wanna put words into his mouth. Although, once again it's not clicking "edit source", it's clicking the file then the tab "code", which is available for all markdown files to see the original code instead of the markdown render.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The intent of this is not to show it nicely, but to make it easy to copy into your PR. So if we want to keep it (which I still doubt the usefulness of), it should be turned into a code block. If you show it "nicely", many users will just be very confused about why they can't click the check boxes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed it completely 👍 not sure it's useful either



[1]: https://docs.datadoghq.com/help
[2]: https://github.com/DataDog/dd-trace-js/blob/master/index.d.ts
[3]: https://github.com/DataDog/dd-trace-js/blob/master/docs/test.ts
[4]: https://github.com/DataDog/documentation/blob/master/content/en/tracing/trace_collection/library_config/nodejs.md
[5]: https://github.com/DataDog/dd-trace-js/blob/master/.github/workflows/plugins.yml
Loading