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

Added command line flag parsing via cli package #13

Closed
wants to merge 2 commits into from
Closed

Added command line flag parsing via cli package #13

wants to merge 2 commits into from

Conversation

langston-barrett
Copy link
Contributor

Progress on #7
Fixes #12, fixes #1.

This is a great example of why you should use external libraries instead of falling trap to NIH.

@dborzov
Copy link
Owner

dborzov commented Jul 10, 2015

Nice! Thanks!

You seem to have accidentally committed the executive binary for lsp. Could you clean that up?

@langston-barrett
Copy link
Contributor Author

Done, and put in .gitignore for future builds.

@langston-barrett
Copy link
Contributor Author

@dborzov Do you need anything else?

@dborzov
Copy link
Owner

dborzov commented Jul 14, 2015

@siddharthist sorry for the delay. PR looks great and is sorely needed! Thanks!

A couple of things:

  • lsp --help prints out a nice help message but then the list of files is printed for the current directory right after it. It looks like this:
Incorrect Usage.

NAME:
   lsp - lsp is like ls command but more human-friendly

USAGE:
   lsp [global options] [arguments...]

VERSION:
   dev

AUTHOR(S):
   dborzov

GLOBAL OPTIONS:
   -d, --directories    Only show directories
   -h, --human      Show human readable file sizes
   -l, --long       Long form, show more details
   -s, --size       Show and order by size
   -t, --time       Show and order by modification time
   -p, --pyramid    Align files to the left side
   --version, -v    print the version

-------------------------------------------------------------------------------------------------
                                         fff333  dir(empty one)
                                    .travis.yml  Text File
                                     .gitignore  Text File
                                   arguments.go  Text File
                              arguments_test.go  Text File
                                CONTRIBUTING.md  Text File
                                         fmt.go  Text File
                                    fileinfo.go  Text File
                                    filelist.go  Text File
                                 investigate.go  Text File
                                        LICENSE  Text File
                                        main.go  Text File
                                       paths.go  Text File
                                      README.md  Text File
                                        sort.go  Text File
                                     tty_win.go  Text File
                                    traverse.go  Text File
                                        trie.go  Text File
                                    tty_unix.go  Text File
                                       utils.go  Text File
-------------------------------------------------------------------------------------------------
lsp "/k/src/github.com/dborzov/lsp", 21 files, 1 directories
               git repo (remote at [email protected]:dborzov/lsp.git)

It should be only the help message.

  • Target directory argument should be mentioned in the help message. As it is processed outside cli currently
    lsp -s /k/src/github.com/dborzov/ is broken, for example. Maybe reuse codegansta/cli's tools for it as well?
  • I would not put lsp into .gitignore. The binary file has been committed and then deleted in the next one, so it is still cached by git. Maybe rewrite history to just not commit the binary at all?
  • Maybe put the documentation edits and go fmt some of the files in separate dedicated commits?
  • I would delete -d, -h, -l, -p keys as they are not implemented anyway
  • Plz delete Author and Version sections for lsp --help. I don't think anyone needs them.

I can add any of those changes myself if you prefer.

@langston-barrett
Copy link
Contributor Author

@dborzov I'd be happy to add some of them, I've just been a little busy. Feel free to add any yourself as well.

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.

Create --help flag Help option
2 participants