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

option to edit dotfile #1

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

option to edit dotfile #1

wants to merge 3 commits into from

Conversation

creiber
Copy link

@creiber creiber commented Jun 25, 2020

  • option to edit dotfile
  • no error message when called without args

- option to edit dotfile
- no error message when called without args
Copy link
Owner

@rse rse left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, Christian! See below for my comments.

}
if [ $# -eq 0 ]; then
echo "dotfiles: ERROR: invalid number of arguments" 1>&2
Copy link
Owner

Choose a reason for hiding this comment

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

Why are you removing this line? It seems to be not unrelated to the new -e options.

Copy link
Author

Choose a reason for hiding this comment

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

This is true but IMHO shell tools are expected to emit their usage either when called without arguments or when called with an explicit –h or –help option. One of those should work and is not an error. It is just the user asking for advice how to use the tool. YMMV.

dotfiles.sh Outdated
@@ -14,9 +14,9 @@ srcdir="@datadir@"
# command-line argument parsing
usage () {
echo "dotfiles: USAGE: dotfiles [-v|--version] [-q|--quiet] [-f|--force] [-p|--preserve] [-r|--remote <user>@<host>] <home-directory>" 1>&2
echo " dotfiles -e [<file>...]" 1>&2
Copy link
Owner

Choose a reason for hiding this comment

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

Here you should write -e|--edit to also show the long-option alias.

@@ -31,6 +31,7 @@ while [ $# -gt 0 ]; do
-q|--quiet ) quiet=yes; shift ;;
-f|--force ) force=yes; shift ;;
-p|--preserve ) preserve=yes; shift ;;
-e|--edit ) edit=yes; shift ;;
Copy link
Owner

Choose a reason for hiding this comment

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

You should pre-initialize the variable edit beforehand similar to what is done for the other options.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@@ -39,8 +40,13 @@ if [ $version = yes ]; then
echo "$versionstr"
exit 0
fi
if [ $edit = yes ]; then
cd $HOME/.dotfiles
exec ${EDITOR:-vi} ${*:-.}
Copy link
Owner

Choose a reason for hiding this comment

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

Not all $EDITORs might support editing a directory .. This then would fail here...

Copy link
Author

Choose a reason for hiding this comment

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

Correct. The user is free to either provide a capable editor in $EDITOR or to explicitely name the file he or she wants to edit. If they refuse to provide the prereqs they won’t receive the benefits. That's something to be documented but I would not miss the fine feature of directory selection as most editors in use today (and preinstalled) can do it.

if [ $# -ne 1 ]; then
echo "dotfiles: ERROR: invalid number of arguments" 1>&2
echo "dotfiles: ERROR: invalid number of arguments $#" 1>&2
Copy link
Owner

Choose a reason for hiding this comment

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

It's OK to print the number of actually provided arguments, but (a) it doesn't read nicely when you see "invalid number of arguments 7" and (b) the actual number of expected arguments could be more useful in the output. Perhaps a better error could be "invalid number of arguments (expected 1 argument, received N arguments)".

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.

2 participants