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

Prevent repeat dotfile edits and a more pleasant UX #82

Closed
wants to merge 4 commits into from

Conversation

adarsh
Copy link

@adarsh adarsh commented Apr 5, 2013

Updates

  • Remove check for SSH key before copying. This shouldn't fail unless the previous keygen step fails, in which case this doesn't matter
  • Check for existing dotfile changes before repeating them. Ensures idempotency of the script. Applies to Homebrew path priority change and rbenv init call.

UX

  • Add intro and pause at beginning (e.g. 'hit return to start')
  • Clearer description of browser-opening step. Browser opening can surprise people. This gives you some context.
  • Add trailing echo's to separate output
  • Add pause and instructions for setting up dbs at boot
  • Give a 'hit return to continue' to give users time to read
  • Instruct them on how to set up DBs to start at boot

Adarsh Pandit and others added 2 commits April 5, 2013 14:37
Updates
-------

* Remove check for SSH key before copying. This shouldn't fail unless the previous keygen step fails, in which case this doesn't matter
* Check for existing dotfile changes before repeating them. Ensures idempotency of the script. Applies to Homebrew path priority change and rbenv init call.

UX
--

* Add intro and pause at beginning (e.g. 'hit return to start')
* Clearer description of browser-opening step. Browser opening can surprise people. This gives you some context.
* Add trailing echo's to separate output
* Add pause and instructions for setting up dbs at boot

* Give a 'hit return to continue' to give users time to read
* Instruct them on how to set up DBs to start at boot
* [Avoid captive user interfaces][1].
* [Don't be chatty][2].
* Move `exec $SHELL -l` line to spot recommended by [rbenv README][3].

[1]: http://goo.gl/cocdC
[2]: http://goo.gl/mL7hn
[3]: https://github.com/sstephenson/rbenv/
@croaky
Copy link
Contributor

croaky commented Apr 6, 2013

@adarshpandit Thanks for the PR. I really like the changes to make it idempotent. Also good call on the SSH check.

I pushed a follow-up commit to this branch that:

  • Removes the extra echos.
  • Does not print anything to stdout.
  • Removes the captive user interface changes.
  • Adds a line about idempotence to the README.
  • Removes the printed instructions about Postgres and Redis.

I provided links in the commit explaining why I don't want the script to be chatty or create a captive user interface.

I'd rather see the Postgres and Redis commands just run, rather than printed as instructions. A goal of the script is to be a one-liner. If there are commands from those brew infos that we should bring in, those would be good separate PRs.

Take a look at the current diff and let me know what you think.

@Kerrick
Copy link
Contributor

Kerrick commented Apr 7, 2013

Moved the logging/stdout part of this post to #83.

I completely agree with avoiding a captive user interface in most situations. However, I totally get why @adarshpandit added them in the first place--if you're not actively watching the text stream by, you can totally miss the fact that your clipboard was modified and the reason the browser just opened. In fact, in your follow-up commit, those are completely omitted.

In fact, an argument could be made that a captive UI is a good thing in this situation, because the script (being run in the terminal) is modifying your clipboard, potentially overwriting something important. It's a good idea to warn the user before that happens so they can paste the information somewhere.

Perhaps a good compromise could be to move the SSH key generation/copying and the GitHub opening to the end, mention them to the user, and halt the script only for the clipboard alteration?

@croaky
Copy link
Contributor

croaky commented Apr 7, 2013

However, I totally get why @adarshpandit added them in the first place--if you're not actively watching the text stream by, you can totally miss the fact that your clipboard was modified and the reason the browser just opened.

Definitely. Taking another approach, I've removed the "copy SSH key to clipboard" bit in 3876ff5.

I opened a separate issue for logging the information here:

#83

Definitely interested in more opinions about status indicators printing out vs. just logging.

@adarsh
Copy link
Author

adarsh commented Apr 8, 2013

Very interesting approach and well worth the discussion. I prefer the version I submitted because I find it much easier to use and understand. Removing all standard output feels like a pretty big change, hence the lengthy discussion below.

The script installs and modifies many programs and libraries for you. I think it's important to know specifically what it does (and maybe why). For example, you might try to alter your path and wonder why it's been changed in the way it has (or even how it got changed, if you didn't know laptop did it for you). I'd bet more than a minority of users run the script without reading it - I know I've run scripts without reading them all the way through.

The references to UNIX philosophy are very interesting. One downside to Captive User Interfaces noted is parsing the prompted response. In this case, we're not parsing responses, only pausing for the user to read about the big changes we're making to their environment.

To me, this script doesn't feel as much like a "UNIX program", like grep or ack as much as it does a "script" or "macro". I might have the wrong idea completely about UNIX but it definitely feels like a different beast. Silver Searcher feels closer to the UNIX family than this does.

Now that may mean it should be made to be more like a UNIX program (closer to your edits) or that scripts like this which replicate repetitive command-line statements fall under some different concept. I'm not sure I have the answer for that.

Practically, I would think not seeing progress/output would make me wonder if the script was working at all. This script has stalled/failed on me a number of times and knowing where is important as I debug. I could see using this on a friend's computer and have them look at me and ask if it was running.

The underlying programs, the OS X environment, and user customization are all sources of instability and change. I can't say this script works 100% of the time given the uncontrollable shiftiness I mentioned. As such, I'd like a lot more detail on what fails and how far along we've gotten before it does. @croaky I'm curious to know if you've noticed this brittleness as well.

Also worth noting, the successfully method, which should give nice errors on failure, doesn't always exit the script upon failures. I'm guessing not all of those methods return the right success code, but that's just a guess.

I kind of liked having the script copy my key and open the browser, but maybe I'm in the minority. If it's kept, I still think it's worth warning the user before opening a browser.

Anyhow, I'm not big on the changes and agree with @Kerrick for the most part, although I'd prefer seeing std out progress over an output logfile, especially if it's run repeatedly. I think the output serves as a learning tool as well - I first learned about Redis from this script. That being said, I'm curious to hear other thoughts about these changes.

At the very least, I've already learned a ton through this discussion and now I'm ordering a copy of "The UNIX Philosophy".

@adarsh
Copy link
Author

adarsh commented Apr 8, 2013

Not sure if this is too late, but a change this big might be best in a separate PR, so this one would be "Improve what we already have" while the new one would be "Remove all stdout". Just a thought

@ghost ghost assigned croaky Apr 9, 2013
@croaky
Copy link
Contributor

croaky commented Apr 9, 2013

a change this big might be best in a separate PR

Agree these should be broken out. Going to separate them and try one other approach to output.

@croaky
Copy link
Contributor

croaky commented Apr 9, 2013

Removing all standard output feels like a pretty big change

My earlier change didn't remove all standard output, although I see that's how I phrased it. So, my apologies. All I was doing was removing the custom output, not the output from the scripts themselves, which print quite a bit. I don't think that was serving much of a purpose and I think the arguments about users thinking the script is hanging are valid.

So, I extracted the pieces from our commits into smaller PRs:

And added a new one, Print commands as they execute, to add a bit more context to the output.

I don't think I want to add the reads right now for fear of users thinking the script is done. As a compromise, I added a little bit of extra documentation encouraging the user to read the script before running it: 70bcd8f

Let me know what you think and thanks for all the ideas, the pull request, and comments.

@croaky croaky closed this Apr 9, 2013
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.

3 participants