Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Js getting started more updates #2652

Merged
merged 21 commits into from
May 9, 2023

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented May 2, 2023

This PR #2651 and #2653 create some consistency across python, java & Node.JS documentation.

Preview:
https://deploy-preview-2652--opentelemetry.netlify.app/docs/instrumentation/js/getting-started/nodejs/

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

See inline comments.

Here's another suggested change. From:

One tool commonly used for this task is the
[`-r, --require module`](https://nodejs.org/api/cli.html#cli_r_require_module)
flag. x

To:

One tool commonly used for this task is the
[--require](https://nodejs.org/api/cli.html#-r---require-module)
flag.

Note the corrected URL and hash.

<details>
<summary>View example output</summary>

```javascript
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
```javascript
```json

Copy link
Member Author

Choose a reason for hiding this comment

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

the output is not json, so some editors scream at you in red when looking at that.

I am OK with both.

Copy link
Member

Choose a reason for hiding this comment

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

maybe there is a jsonlines option?

Copy link
Contributor

Choose a reason for hiding this comment

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

@svrnm, maybe try jsonlines (I'd never tried that one before). If it doesn't work go with which ever option you prefer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I put in jsonlines, also not sure if this is a legal argument although it is not screaming at me in red anymore in VSCode, I would say that's a win

@cartermp
Copy link
Contributor

cartermp commented May 5, 2023

@dyladan any further feedback?

<details>
<summary>View example output</summary>

```javascript
Copy link
Member

Choose a reason for hiding this comment

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

maybe there is a jsonlines option?

svrnm and others added 17 commits May 8, 2023 07:50
Signed-off-by: svrnm <neumanns@cisco.com>
Signed-off-by: svrnm <neumanns@cisco.com>
Signed-off-by: svrnm <neumanns@cisco.com>
Signed-off-by: svrnm <neumanns@cisco.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Signed-off-by: svrnm <neumanns@cisco.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Signed-off-by: svrnm <neumanns@cisco.com>
Signed-off-by: svrnm <neumanns@cisco.com>
@chalin chalin force-pushed the js-getting-started-more-updates branch from 6c7ce5d to 1a736d8 Compare May 8, 2023 11:50
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

One final tweak, otherwise LGTM

svrnm and others added 2 commits May 8, 2023 15:02
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
@cartermp cartermp merged commit 61b54bf into open-telemetry:main May 9, 2023
8 checks passed
@svrnm svrnm deleted the js-getting-started-more-updates branch May 11, 2023 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants