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

pages/common/*: replace unportable shell syntax in command examples #12848

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

tricantivu
Copy link
Member

@tricantivu tricantivu commented May 27, 2024

  • The page(s) are in the correct platform directories: common, linux, osx, windows, sunos, android, etc.
  • The page(s) have at most 8 examples.
  • The page description(s) have links to documentation or a homepage.
  • The page(s) follow the content guidelines.
  • The PR title conforms to the recommended templates.
  • Version of the command being documented (if known):

As proposed in #12694, I think the command examples of pages in the directory common must should adhere to the latest POSIX standard to have some degree of certainty that the commands will function properly in many shells.

To find pages with unportable syntax, I used the following:

#!/usr/bin/env bash

codes=(SC{3000..4000})

for f in pages/common/*.md; do
    echo "Page: ${f}"
    grep -Ex '`.+`' -- "${f}" | sed 's/`//g' | shellcheck -i "$(echo "${codes[@]}" | tr " " ",")" -s sh -f gcc -
done
  • Process substitution:
grep -FR --exclude-dir=pages.en '<(' pages*
  • Extended globs:
grep -RE --exclude-dir=pages.en '[?*+@!]\(.*\)' pages*
  • function keyword
grep -RE --exclude-dir=pages.en 'function [A-Za-z0-9_]+(\(\))?' pages*
  • "File slurps" 1:
grep -RExl --exclude-dir=pages.en '`.*\$\(<.*\).*`' pages*

It is worth noting that the grep commands and the script are nowhere near foolproof, I encountered many false positives, especially about brace expansions. Therefore, there are probably more pages out there with unportable shell syntax.

Footnotes

  1. https://mywiki.wooledge.org/Bashism#Syntax

@github-actions github-actions bot added the page edit Changes to an existing page(s). label May 27, 2024
Copy link
Member

@gutjuri gutjuri left a comment

Choose a reason for hiding this comment

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

Thanks! Even though this is still WIP I have added some comments. I think that we should proceed very carefully to preserve the original meaning of the examples.

pages/common/comm.md Outdated Show resolved Hide resolved
pages/common/for.md Outdated Show resolved Hide resolved
pages/common/git-filter-repo.md Outdated Show resolved Hide resolved
@@ -29,4 +29,4 @@

- Show the difference between two `sops` files:

`diff <(sops -d {{path/to/secret1.enc.yaml}}) <(sops -d {{path/to/secret2.enc.yaml}})`
`diff {{path/to/sops_file1}} {{path/to/sops_file2}}`
Copy link
Member

Choose a reason for hiding this comment

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

Diff doesn't work for encrypted files. The sops -d command must be called to decrypt the sops files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would replacing the placeholder for {{decrypted_sops_file}} cut it?

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion not, because decrypted sops files are just plain json or yaml files if I understand the docs of sops correctly.

Also, I do not think that we should eliminate process substitution from our examples. I highly doubt that any user uses a shell that doesn't support process substitution as their daily driver (no beginner user, anyway). I think that tldr pages remain more useful with the process substitution examples left in.

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion not, because decrypted sops files are just plain json or yaml files if I understand the docs of sops correctly.

Also, I do not think that we should eliminate process substitution from our examples. I highly doubt that any user uses a shell that doesn't support process substitution as their daily driver (no beginner user, anyway). I think that tldr pages remain more useful with the process substitution examples left in.

SOPS is an editor of encrypted files that supports YAML, JSON, ENV, INI and BINARY formats [...]
from here
so the decrypted sops file is in one of the formats above.

pages/common/tee.md Outdated Show resolved Hide resolved
@vitorhcl vitorhcl changed the title pages/common/*: replace command examples with unportable shell syntax pages/common/*: replace unportable shell syntax in command examples Jun 4, 2024
@spageektti
Copy link
Member

Is this PR ready to review or still WIP?

@tricantivu
Copy link
Member Author

Is this PR ready to review or still WIP?

The latter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
page edit Changes to an existing page(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants