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

chore(docs): remove $ sign from shell commands #458

Merged
merged 1 commit into from Jun 24, 2022
Merged

chore(docs): remove $ sign from shell commands #458

merged 1 commit into from Jun 24, 2022

Conversation

nhedger
Copy link
Contributor

@nhedger nhedger commented Jun 14, 2022

This PR removes the $ sign from the bash code blocks to allow copy/pasting their content as is.

@SimonFrings
Copy link
Member

Hey @nhedger, thanks for bringing this up 👍

I agree with you that this is way more comfortable to copy and paste these single-lined commands. This fits ReactPHP's HTTP component perfectly, because it doesn't contain any multi-lined commands inside its README. The same adjustments should also be made for ReactPHP's other components (same case).

As we're also maintaining other projects on GitHub (like Framework X etc.) we have to look at the bigger picture here. While this makes sense for single-lined commands I'm wondering if we should apply the same changes for multi-lined ones. For example:

$ curl http://localhost:8080/
Hello wörld!

or

$ php examples/index.php
$ tests/acceptance.sh http://localhost:8080

The first example shows the executable command with its following output, removing the $ could lead to some misunderstanding in some more complex examples. The second one shows two commands in one block which could also lead to some confusion (also the case for one command that takes up multiple lines).

I don't want to bloat the whole topic, but please understand that we care about consistency across our multiple projects.

I am curious what you think about this.

@WyriHaximus
Copy link
Member

For example:

$ curl http://localhost:8080/
Hello wörld!
curl http://localhost:8080/

Outputs:

Hello wörld!

@nhedger
Copy link
Contributor Author

nhedger commented Jun 15, 2022

Hey,

Wasn't expecting a long answer for such a small change but I understand your concern so I'll try to explain my point of view.

When a code block is annotated as bash or shell, I expect to find only bash or shell commands inside, respectively. In this instance, the $ sign is not a part of the command, it is merely used as a visual indicator, whether you are using single-line or multi-line commands.

The example output could easily live in another code block that is not annotated, right after.

But to be fair, this PR isn't even about that, it was just a matter of simplifying the copy/paste experience.

@WyriHaximus
Copy link
Member

a matter of simplifying the copy/paste experience.

And this is why I'm in favour of it. What's you POV @clue ?

@clue clue added this to the v1.7.0 milestone Jun 20, 2022
@clue
Copy link
Member

clue commented Jun 20, 2022

@nhedger Thank you for this PR and sparking the discussion!

Wasn't expecting a long answer for such a small change but I understand your concern […]

I agree the $ can be a small nuisance when copy-pasting and also agree with everything @SimonFrings said why we've been using this for years despite this: The $ is a clever indicator to separate command line input from other commands or command line output. Looking at it now, I don't think using such subtle hints is particularly obvious to the average developer and might actually cause more harm than good.

As such, I agree that we can safely remove this from all our docs. I'd like to make sure we do so consistently across all our components, so I'd love to see if anybody feels like picking this up and files a bunch of PRs linked against this original PR 👍

It's currently my understanding we can safely apply this to all single-line commands. Any code fences with multiple commands or commands with outputs may need a slightly bigger change to possibly split this into multiple code fences as suggested above or we may want to keep using the $ here as copy-pasting is much less of an issue for such code blocks anyway.

@nhedger
Copy link
Contributor Author

nhedger commented Jun 20, 2022

@WyriHaximus
Copy link
Member

Thank you for the great effort @nhedger ! Merging this as most of your other PR's also have been merged at this point and the last 3 should be take long either.

@WyriHaximus WyriHaximus merged commit b3ff9c8 into reactphp:1.x Jun 24, 2022
@nhedger nhedger deleted the chore/improve-readme branch June 24, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants