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

Update StyleGuide.md to note preference for curl over wget #18479

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

Conversation

mdlinville
Copy link
Contributor

Relates to #18477

Copy link

Files changed:

  • StyleGuide.md

Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for cockroachdb-interactivetutorials-docs canceled.

Name Link
🔨 Latest commit 7aad0c4
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-interactivetutorials-docs/deploys/661d874ce0227800086067b7

Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for cockroachdb-api-docs canceled.

Name Link
🔨 Latest commit 7aad0c4
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-api-docs/deploys/661d874c221e6200084f9a02

Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for cockroachdb-api-docs canceled.

Name Link
🔨 Latest commit a3c0ef9
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-api-docs/deploys/6639448291914f0008098f81

Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for cockroachdb-interactivetutorials-docs canceled.

Name Link
🔨 Latest commit a3c0ef9
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-interactivetutorials-docs/deploys/663944825fd5b70008baeb5c

Copy link

netlify bot commented Apr 15, 2024

Netlify Preview

Name Link
🔨 Latest commit 7aad0c4
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-docs/deploys/661d874ce6a5ad000744df2a
😎 Deploy Preview https://deploy-preview-18479--cockroachdb-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdlinville mdlinville requested a review from mikeCRL April 15, 2024 20:05
Copy link

netlify bot commented Apr 15, 2024

Netlify Preview

Name Link
🔨 Latest commit a3c0ef9
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-docs/deploys/66394482614e4d000867b743
😎 Deploy Preview https://deploy-preview-18479--cockroachdb-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

In an example command to download an artifact using the command line, `curl` is preferred over `wget` because it is included in Linux, macOS, and recent builds of Windows. Use syntax like:

```
curl -o {optional_output_path}/{output_file_name} [-H {header_keys_and_values}] {url}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear regarding which of these vars would be printed verbatim in the docs, vs. replaced in the docs with context-specific values, vs. a case by case decision on this. Worth any clarification here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By (Linux man page) convention, options in square brackets are optional and I tried to use that convention here. I wanted to give some hints for writers who may be more used to wget than curl.

  • I don't consider -o optional because leaving it off outputs the stream to standard output (including if it's a binary), for a very ugly experience, so I didn't surround it with square brackets. wget has a behavior difference here: By default, a binary is always downloaded, and the download has the same file name as the file portion of the download URL.
  • I added the {optional_output_path} part to the -o argument to show how to specify a local download path with curl if you are used to passing -P to wget to designate a path. I was trying to show that you don't need an extra argument to specify the path in curl, just reference it.
  • We have some CC API examples where explicitly pass in headers, so I wanted to give a clue about how you would do that with curl's -H argument, but I didn't want to imply that the header is always required.
  • I don't consider {url} to be optional as it shows what to download or what REST URL to hit.

Comment on lines +830 to +832
- If you do not specify `-o` or `--output`, the URL's contents are printed to standard output, even if it is a binary file. This can corrupt your terminal session and is useful only when piping or redirecting the output to a subsequent command.
- If you do not include a path in your output file name, the file is saved to the current working directory.
- Headers are optional, but certain APIs such as the CockroachDB Cloud API require certain header fields to be set.
Copy link
Contributor

Choose a reason for hiding this comment

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

These are great points and good to know.

Are these intended as 'reasons why we should always include the parameters above' or 'considerations when deciding whether to include the parameters above'?

Perhaps a single-sentence intro to the list can clarify that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither. I was taking into consideration the different types of wget examples I updated to use curl and to show what to do when their behavior differs, but I wanted to keep it very narrow to the sorts of ways our docs use curl rather than exhaustively document all the options to each command, and to try to prevent you from streaming the download binary to standard output.

  • We had several wget examples that specify an output file name, path, or both.
  • We had several examples where passing extra headers is a key part of the example, so I wanted to show how you'd do that with curl.

Part of the motivation for why I did it this way is that this is very specific info about one specific type of code example, and there isn't a great home for it in the style guide's TOC. I was hoping the info would come up in searches. WDYT?

@mdlinville mdlinville requested a review from mikeCRL April 16, 2024 23:29
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

2 participants