FAQ
Hi all,

I haven't seen any of these in list so probably I'm asking to the wrong
mailing list (or maybe not).

I'm fairly new to go, although during the lasts weeks I've been following
the list and writing small pieces of go, reading docs, etc.
I managed to "convince" my project mates to port a CLI that we did in ruby
to golang as the former was giving some I/O related errors difficult to
overcome, issues with binary distribution, etc..
Before doing some actual work in the program I've written a small option
parser a la git/mercurial/go for practising ( I'll use it eventually). I
would really appreciate if some of you (oh experienced go coders :) )
could spend 5 min and have a look to the code and criticise it. I'm pretty
sure that I may be doing something fishy although I did my best to get the
main concepts used in go.

https://github.com/capitancambio/go-subcommand

Thanks a million!

cheers,

Javi

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Carlos Castillo at Aug 5, 2013 at 10:10 pm
    I haven't looked too closely... yet, but I have used some of the common go
    tools.

    I did see these while skimming though:

        1. The function getFlagLonfDefinition has a typo in the name (at least I
        assume it's not supposed to be "Lonf").
        2. You create lists by starting with an empty slice, and then appending
        elements. This is fine, but you seem to be going about it oddly. To create
        an empty array, v := make([]T, 0) seems odd, although the compiler might
        optimize the do-nothing make out for you, "var v []T", or "v := []T(nil)"
        does the same thing, with less typing, and seems less like a mistake to me.
        However, since you know the final size of the list you are making, then "v
        := make([]T, 0, n)" makes even more sense as it will allow append to be
        efficient both memory-wise and cpu-wise. This isn't serious, but it looks
        like you were unaware of your options.
        3. You are using asserts in your tests, which don't save much effort
        over simply using an if statement with t.Errorf, and you loose the ability
        to make the messages more meaningful and helpful by customizing them to the
        error. Also if you use some table driven tests (see below), you will have
        far less code in your tests, and a personalized error message is even more
        useful (eg: printing failing test case information).


    The following I got out of tools:

        - go vet:
        - Some of your printf formatting is incorrect:
              - Line 55 has Sprintf %s printing a Command value, but that type
              has no String method
              - Line 256 has no formatting characters in the format string for
              Sprintf, but passes variables, you could use fmt.Sprint instead or insert a
              format character.
              - Line 231 in the *test file* makes the inverse mistake, it has
              "%v" in the first argument to t.Error(), when you should be using t.Errorf.
           - golint (external tool at github.com/golang/lint/golint):
           - You use errors.New(fmt.Sprintf(...)) a lot, when you could more
           easily use fmt.Errorf() instead (http://golang.org/pkg/fmt/#Errorf)

    If you are building tip from source (or are from the future), you can also
    use the built-in coverage tester to see places where your tests fail to
    check your code. I used it as follows:

    go test -covermode atomic -coverprofile coverage.dat
    go tool cover -html coverage.dat -o coverage.html

    The output is attached, red lines were never executed by your tests. Dave
    Cheney wrote a nice blog post about how to write table driven tests
    (http://dave.cheney.net/2013/06/09/writing-table-driven-tests-in-go), which
    can help you easily test more aspects of your code.
    On Monday, August 5, 2013 8:57:25 AM UTC-7, Capitan Cambio wrote:

    Hi all,

    I haven't seen any of these in list so probably I'm asking to the wrong
    mailing list (or maybe not).

    I'm fairly new to go, although during the lasts weeks I've been following
    the list and writing small pieces of go, reading docs, etc.
    I managed to "convince" my project mates to port a CLI that we did in ruby
    to golang as the former was giving some I/O related errors difficult to
    overcome, issues with binary distribution, etc..
    Before doing some actual work in the program I've written a small option
    parser a la git/mercurial/go for practising ( I'll use it eventually). I
    would really appreciate if some of you (oh experienced go coders :) )
    could spend 5 min and have a look to the code and criticise it. I'm pretty
    sure that I may be doing something fishy although I did my best to get the
    main concepts used in go.

    https://github.com/capitancambio/go-subcommand

    Thanks a million!

    cheers,

    Javi
    --
    You received this message because you are subscribed to the Google Groups "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Antoine Grondin at Aug 6, 2013 at 3:11 am
    The coverage tool is very nice, I didn't even know it existed as part of
    the (tip) distribution.
    On Monday, August 5, 2013 11:57:25 AM UTC-4, Capitan Cambio wrote:

    Hi all,

    I haven't seen any of these in list so probably I'm asking to the wrong
    mailing list (or maybe not).

    I'm fairly new to go, although during the lasts weeks I've been following
    the list and writing small pieces of go, reading docs, etc.
    I managed to "convince" my project mates to port a CLI that we did in ruby
    to golang as the former was giving some I/O related errors difficult to
    overcome, issues with binary distribution, etc..
    Before doing some actual work in the program I've written a small option
    parser a la git/mercurial/go for practising ( I'll use it eventually). I
    would really appreciate if some of you (oh experienced go coders :) )
    could spend 5 min and have a look to the code and criticise it. I'm pretty
    sure that I may be doing something fishy although I did my best to get the
    main concepts used in go.

    https://github.com/capitancambio/go-subcommand

    Thanks a million!

    cheers,

    Javi
    --
    You received this message because you are subscribed to the Google Groups "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Javier Asensio at Aug 6, 2013 at 11:09 am
    Carlos thanks a million for your comments. I'll fix that typo :).

    Regarding 2, I just tried the first time as v:=make([]T,0) to get a yet
    unknown-sized slice and it worked, but you're completely right "var v []T"
    looks a way better.

    3. Again it was for my inexperience, at the beginning I thought it would be
    a good idea to have convenience methods for asserts, but as you point out,
    you can't get the fail information from them, so I stopped using them
    eventually but I haven't refactored the first tests yet.

    Thanks for the rest of tools related information, I think one of the
    strongest points of golang ( one of many of them ) is the amazing go tool,
    I didn't know about vet and coverage, I'll include them to my workflow
    right now.

    Cheers!




    cheers,

    Javi

    On Tue, Aug 6, 2013 at 4:11 AM, Antoine Grondin wrote:

    The coverage tool is very nice, I didn't even know it existed as part of
    the (tip) distribution.

    On Monday, August 5, 2013 11:57:25 AM UTC-4, Capitan Cambio wrote:

    Hi all,

    I haven't seen any of these in list so probably I'm asking to the wrong
    mailing list (or maybe not).

    I'm fairly new to go, although during the lasts weeks I've been following
    the list and writing small pieces of go, reading docs, etc.
    I managed to "convince" my project mates to port a CLI that we did in
    ruby to golang as the former was giving some I/O related errors difficult
    to overcome, issues with binary distribution, etc..
    Before doing some actual work in the program I've written a small option
    parser a la git/mercurial/go for practising ( I'll use it eventually). I
    would really appreciate if some of you (oh experienced go coders :) )
    could spend 5 min and have a look to the code and criticise it. I'm pretty
    sure that I may be doing something fishy although I did my best to get the
    main concepts used in go.

    https://github.com/**capitancambio/go-subcommand<https://github.com/capitancambio/go-subcommand>

    Thanks a million!

    cheers,

    Javi
    --
    You received this message because you are subscribed to the Google Groups
    "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an
    email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

    --
    You received this message because you are subscribed to the Google Groups "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Carlos Castillo at Aug 7, 2013 at 4:50 am
    Here are some (more) suggestions.

    Use the existing parser I
    The standard library contains a flag package which although not as
    feature-full as other packages, has some nice properties: it's always
    present (in go), it's type safe, its well tested, and it essentially acts
    as a way to assign global variables (in your program) from the command
    line. I find that most programs I write never need more than the features
    provided by this library.

    There are still good reasons for this project to exist, such as using it to
    learn to program in go, but you should probably still learn the ins and
    outs of the built-in flag package. People looking at your future code will
    be more familiar with the standard flag package than yours, and so will
    have a lower barrier to understanding your code.

    Use the existing parser II
    If you look at the stdlib flag package, one obvious difference between it
    and your package is that flag has many entry points in its API to create
    different type of command line options, while in subcommand you have one
    (AddFlag). Granted you only have two types of command line options in your
    package (those that take no arguments, and those that take a string
    argument), but still, your code tells the difference between the two by the
    contents of the "long" string argument to AddFlag. Essentially, you have
    written a parser (albeit a simple one) to configure your command line
    parser. If instead you had 2 entry points:

    AddFlag(long, short, description string, fn func(string)) (f *Flag, error)
    AddOption(long, short, description string, fn func(string)) (f *Flag, error)

    You wouldn't need to parse the long option name to determine the
    difference. As a result, the person writing the code uses the compiler (ie:
    uses its parser) to choose. This will also reduce the amount of code you
    need to test.

    You could alternatively pass an extra bool argument to AddFlag to make this
    distinction.

    Other Notes

        - If all long options are supposed to use "--", and all short options
        use "-", then calls to AddFlag could be further simplified:
           - eg: cmd.AddFlag("verbose", "v", "Say more stuff", somefunc)
        - AddFlag() should panic instead of returning an error
           - Errors created in AddFlag() can only be dealt with if the program
           is fixed (and the stack trace panic gives you can tell you where)
           - It simplifies the use of AddFlag (no check needed)
           - The same arguments apply to changing AddCommand()
        - A Parser is not actually a Command (despite what your comment says),
        They may satisfy the same set of interfaces, but you can't use a Parser
        where a Command is expected
           - If you want, you probably combine them into the same type (is there
           any reason why a command can't have a sub-command?)
           - Even if you don't, you can make an unexported function that can be
           used to set up most of both since Parser contains a
           Command: http://play.golang.org/p/TqMkjJ0A-D
        - The &Type{} syntax (see: the previous example) allows you to do simple
        object definitions (in case you didn't know)


    On Monday, August 5, 2013 8:57:25 AM UTC-7, Capitan Cambio wrote:

    Hi all,

    I haven't seen any of these in list so probably I'm asking to the wrong
    mailing list (or maybe not).

    I'm fairly new to go, although during the lasts weeks I've been following
    the list and writing small pieces of go, reading docs, etc.
    I managed to "convince" my project mates to port a CLI that we did in ruby
    to golang as the former was giving some I/O related errors difficult to
    overcome, issues with binary distribution, etc..
    Before doing some actual work in the program I've written a small option
    parser a la git/mercurial/go for practising ( I'll use it eventually). I
    would really appreciate if some of you (oh experienced go coders :) )
    could spend 5 min and have a look to the code and criticise it. I'm pretty
    sure that I may be doing something fishy although I did my best to get the
    main concepts used in go.

    https://github.com/capitancambio/go-subcommand

    Thanks a million!

    cheers,

    Javi
    --
    You received this message because you are subscribed to the Google Groups "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Luzon83 at Aug 7, 2013 at 6:16 am
    Some style tips:

    1) You can write

    command = &Command{
    Name: name,
    innerFlagsShort: make(map[string]*Flag),
    innerFlagsLong: make(map[string]*Flag),
    fn: fn,
    }

    instead of

    command = new(Command)
    command.Name = name
    command.innerFlagsShort = make(map[string]*Flag)
    command.innerFlagsLong = make(map[string]*Flag)
    command.fn = fn

    Same for the creation and initialization of 'parser' in NewParser.

    2) Method receivers in Go usually have one letter names. So it would be

    func (p *Parser) AddCommand(...) {}
    func (c *Command) AddFlag(...) {}

    instead of

    func (parser *Parser) AddCommand(...) {}
    func (command *Command) AddFlag(...) {}

    3) Accessor methods in Go usually do not have a 'Get' prefix, so it would
    be X() instead of GetX().

    --
    You received this message because you are subscribed to the Google Groups "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Javier Asensio at Aug 9, 2013 at 2:18 pm
    Thanks both of you for the feedback.

    Carlos:

    The reason why I'm not using the standard parsing package is that this
    library is going to be used within a port from other CLI program written in
    ruby. I need that the transition from the previous tool to the new one to
    be seamless for the users, and, unfortunately, there exist some small
    differences in the argument syntax.

    I'll definitely use two different entry points to add flags, my first
    approach of parsing the long option for making the difference is quite
    stupid, as you kindly pointed out ;). This also applies for how the long
    and short options are processed.
    A Parser is not actually a Command (despite what your comment says),
    They may satisfy the same set of interfaces, but you can't use a Parser
    where a Command is expected

    You're right, still struggling with the OO concepts that I shouldn't apply
    in go.
    The reason for not to allowing sub-commands is just simplicity at this
    stage, that could be included in the future though.

    Thanks a million again, I'll also take into account the &Type{} syntax for
    the object creation and the other style tips given by luzon.




    cheers,

    Javi

    On Wed, Aug 7, 2013 at 7:16 AM, wrote:

    Some style tips:

    1) You can write

    command = &Command{
    Name: name,
    innerFlagsShort: make(map[string]*Flag),
    innerFlagsLong: make(map[string]*Flag),
    fn: fn,
    }

    instead of

    command = new(Command)
    command.Name = name
    command.innerFlagsShort = make(map[string]*Flag)
    command.innerFlagsLong = make(map[string]*Flag)
    command.fn = fn

    Same for the creation and initialization of 'parser' in NewParser.

    2) Method receivers in Go usually have one letter names. So it would be

    func (p *Parser) AddCommand(...) {}
    func (c *Command) AddFlag(...) {}

    instead of

    func (parser *Parser) AddCommand(...) {}
    func (command *Command) AddFlag(...) {}

    3) Accessor methods in Go usually do not have a 'Get' prefix, so it would
    be X() instead of GetX().

    --
    You received this message because you are subscribed to the Google Groups
    "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an
    email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

    --
    You received this message because you are subscribed to the Google Groups "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-nuts @
categoriesgo
postedAug 5, '13 at 3:57p
activeAug 9, '13 at 2:18p
posts7
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase