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

Modify README to make script execution commands clearer #496

Closed
wants to merge 2 commits into from

Conversation

Charliemowood
Copy link
Contributor

It is not overly clear that the commands need to run separately or where they should be run. This minor modification hopefully makes it more clear.

@gohanlon
Copy link
Contributor

I can see how someone could mistakenly attempt to run all three of these commands at once — good catch!

Knowing that each of these commands need to be run individually requires familiarity with curl, less and sh. There‘s a more important implication, though: that users may attempt to install Laptop without first reading the install script.

Laptop‘s install could easily be a single command, and, in fact it used to be. Instead, Laptop encourages a super-important practice: reading and understanding scripts before running them. Maybe the README should do more to encourage this?

I like @Charliemowood‘s proposed change, but, in addition, I‘d favor reworking the "download, review, then execute" annotation to encourage the best practice even more strongly:

Install

Download the script:

curl --remote-name https://raw.githubusercontent.com/thoughtbot/laptop/master/mac

Review the script (never run a script you haven‘t read!):

less mac

Execute the downloaded script:

sh mac 2>&1 | tee ~/laptop.log

@Charliemowood
Copy link
Contributor Author

@gohanlon I really like the changes you have made and having a clear warning. I would support your improved version with a warning and really explaining what each command does. I think having a review step makes a lot of sense and a single command would not be a good result for the users as it would encourage them to run the script without thinking about it.

There is the option of some nice markdown with a clearer warning. Something like:

Note:

It is very important that you read through every script that you run on your operation system. Scripts can make make major changes that you did not intend. Please review this script carefully before running this script on your system.

It might also be nicer to really explain what is meant by curl, less and breaking down what the sh mac 2>&1 | tee ~/laptop.log for users who are not so Bash savvy. I will reflect on it and add a slightly more detailed explanations while hopefully not overly cluttering the README.

@Charliemowood
Copy link
Contributor Author

Charliemowood commented Jul 14, 2017

Based on @gohanlon's suggestions I have expanded the installation instructions hopefully now the installation instructions are more explicit and clear to user as well as encouraging a more thorough review of the script.

Here is a preview of the amended README.
https://github.com/Charliemowood/laptop/tree/patch-1

croaky pushed a commit that referenced this pull request Jul 14, 2017
@croaky
Copy link
Contributor

croaky commented Jul 14, 2017

Thanks, @Charliemowood and @gohanlon! I've rebased and merged as 4dcfacf.

It uses a version of these instructions that split up the steps into three as suggested, adds the "why" statement of the less review step, but keeps the overall instructions short.

@croaky croaky closed this Jul 14, 2017
@croaky
Copy link
Contributor

croaky commented Jul 14, 2017

I also added an optional step to review the log.

gerardo pushed a commit to gerardo/laptop that referenced this pull request May 26, 2018
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

3 participants