FAQ
https://github.com/pierrre/archivefile

It's just a helper for the "archive" package that allows to (un)archive
file/directory to/from file/writer/reader.

Currently, only "archive/zip" is supported.

It is used in https://github.com/pierrre/mangadownloader for .cbz support.

--
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 Sep 23, 2013 at 1:55 am
    Some suggestions:

        1. As you found out ioutil.ReadDir() only returns the immediate contents
        of the given directory, not the full tree of the given directory. If you
        use filepath.Walk instead you can simplify your code quite a bit.
        2. Your method for displaying progress could use some work, it's very
        easy for a user to write code which will hang, or produce an error, and the
        cases that use it properly are far more complicated than ones that don't.
        You might want to consider a different API:
           - Passing a func(string) callback as progress makes it easy to use in
           all cases, and harder to get wrong. It's how filepath.Walk works.
           - Make the whole operation an object with a state that is used in a
           loop (one iteration per file processed), similar to
           how http://golang.org/pkg/bufio/#Scanner works. This provides several
           benefits:
              - Better control for programmers using the code, as they can
              decide themselves when to proceed, and possibly allow them to cancel the
              operation.
              - Easier to add features without breaking backwards compatibility
              (eg: add new methods to query per-iteration)

    Here is an example of how the above can be used to simplify the code, and
    make a simple zip replacement program: http://play.golang.org/p/zvc7p2Tqna
    On Friday, September 20, 2013 4:56:38 AM UTC-7, Pierre Durand wrote:

    https://github.com/pierrre/archivefile

    It's just a helper for the "archive" package that allows to (un)archive
    file/directory to/from file/writer/reader.

    Currently, only "archive/zip" is supported.

    It is used in https://github.com/pierrre/mangadownloader for .cbz support.
    --
    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.
  • Pierre Durand at Sep 23, 2013 at 11:45 am
    Thank you!

    I had not realized that filepath.Walk() worked recursively.
    I'll use it.

    A function is easier to use than a channel.
    I'll create my own type, like WalkFunc.

    I don't understand "Make the whole operation an object".
    In you example, you create a new zip writer.
    It defeats the purpose of my library :P (it's harder to use)

    Le lundi 23 septembre 2013 03:55:05 UTC+2, Carlos Castillo a écrit :
    Some suggestions:

    1. As you found out ioutil.ReadDir() only returns the immediate
    contents of the given directory, not the full tree of the given directory.
    If you use filepath.Walk instead you can simplify your code quite a bit.
    2. Your method for displaying progress could use some work, it's very
    easy for a user to write code which will hang, or produce an error, and the
    cases that use it properly are far more complicated than ones that don't.
    You might want to consider a different API:
    - Passing a func(string) callback as progress makes it easy to use
    in all cases, and harder to get wrong. It's how filepath.Walk works.
    - Make the whole operation an object with a state that is used in a
    loop (one iteration per file processed), similar to how
    http://golang.org/pkg/bufio/#Scanner works. This provides several
    benefits:
    - Better control for programmers using the code, as they can
    decide themselves when to proceed, and possibly allow them to cancel the
    operation.
    - Easier to add features without breaking backwards
    compatibility (eg: add new methods to query per-iteration)

    Here is an example of how the above can be used to simplify the code, and
    make a simple zip replacement program: http://play.golang.org/p/zvc7p2Tqna
    On Friday, September 20, 2013 4:56:38 AM UTC-7, Pierre Durand wrote:

    https://github.com/pierrre/archivefile

    It's just a helper for the "archive" package that allows to (un)archive
    file/directory to/from file/writer/reader.

    Currently, only "archive/zip" is supported.

    It is used in https://github.com/pierrre/mangadownloader for .cbz
    support.
    --
    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 Sep 23, 2013 at 3:34 pm
    I didn't put an example of the "operation as object" in my code. I pointed
    at the bufio.Scanner object as example of that pattern. I just meant by my
    second point that you might want to reconsider the API, and gave two
    alternatives, a simple one for you to try (callbacks), as well as a
    completely different one (resulting in a lot of refactoring) that would
    offer some new benefits.

    My main point was to show that filepath.Walk already does most of what you
    tried to re-create with ioutil.ReadDir and 3 recursive functions, and that
    your method for returning progress could be improved.

    What may have confused you was the code at the top, the Archiver interface,
    which I created to show how you can separate the zip-specific functionality
    from the code. Here is how the code looks without
    it: http://play.golang.org/p/yCshfPjO2W

    Some things to consider for your current and future packages:

        1. Channels are a nifty new feature of Go, but great care should be used
        when putting them into the exported API of a package. In some cases, like
        your old code, they require the user of your package to create parallel
        code to use it properly which complicates their side greatly, and can have
        some unfortunate side-effects if used improperly. You fortunately allowed a
        nil progress variable to be passed in, meaning that the user no longer
        needed concurrent code to run your functions, but it means that the code
        that used the progress feature were far more complicated than the code that
        didn't.
        2. In tandem with above, most packages should not force the user to
        create their own parallel go-routines to effectively use them.
        3. The above points don't prevent you from using goroutines/channels
        inside your package, just that having them as part of the API leaves a
        minefield of potential problems for the users of your package, where users
        have to (in the case of the old archivefile/zip) read the code themselves
        to see how the channel is used to understand what they need to do to
        prevent the code from blocking (run a separate go-routine), and how to end
        their progress checking loop (they have to manually do it themselves).
        4. Try not to take this the wrong way: This package isn't very useful.
        For people looking to pack/unpack a zip-file full of files from/to a
        folder, many tools/programs already exist for that (zip, WinZip, 7-zip,
        context menu options built-in to OSX and Windows, etc...). Those that want
        to do it from within a go program will probably just use archive/zip
        directly themselves, since it has far fewer restrictions than your code,
        which is built for a very specific set of cases. With only a few dozen
        lines, I was able to re-create the functionality of your package that I
        cared about, the big difference being that it's my code so I understood it
        better, and could then modify it to do additional tasks such as filter
        desired folders/files, pre-process others, or do whatever else I wanted.
        This code will probably still be useful to you, and you can keep it around
        should you need to pack a folder of files into a zip-file as part of a
        larger operation, but most people won't use it enough for it to be useful
        to them to keep it around. Most likely, this package's most important uses
        have already happened:
           1. to implement a feature in mangadownloader
           2. to allow others to critique your coding style to help you learn to
           how to better program in go :-)
        5. In general documentation can almost always be better. It can also
        help if you can't avoid some of the problems above. I also use the API and
        documentation shown on godoc.org as a first impression to see if a package
        is even worth considering.
        6. More tests, including ones that check that error cases fail properly,
        benefit a package's stability.
           - Your current test coverage for archivefile/zip is at 61.5%
           (reported by go 1.2's coverage tool: go test -cover)
           - mangadownloader is at 54%
        7. You should be aware of / consider the output of tools that exist to
        assist in creating better code and assist in finding problems:
           - go vet
           - The race detector (go build -race / go test -race)
           - go test -cover (new in Go 1.2) and it's html output option
           - github.com/golang/lint/golint - for coding style tips (run with:
           "golint ." or "golint pkg" NOT "golint")
           - github.com/remyoudompheng/go-misc/deadcode - to find unused &
           unexported code
           - github.com/kisielk/errcheck - to find cases where errors are ignored
           - Be careful when running the external tools, since they might parse
           their command lines differently then the built-in tools (eg: golint with no
           args does nothing; it produces no output, and no errors)

    On Monday, September 23, 2013 4:43:33 AM UTC-7, Pierre Durand wrote:

    Thank you!

    I had not realized that filepath.Walk() worked recursively.
    I'll use it.

    A function is easier to use than a channel.
    I'll create my own type, like WalkFunc.

    I don't understand "Make the whole operation an object".
    In you example, you create a new zip writer.
    It defeats the purpose of my library :P (it's harder to use)

    Le lundi 23 septembre 2013 03:55:05 UTC+2, Carlos Castillo a écrit :
    Some suggestions:

    1. As you found out ioutil.ReadDir() only returns the immediate
    contents of the given directory, not the full tree of the given directory.
    If you use filepath.Walk instead you can simplify your code quite a bit.
    2. Your method for displaying progress could use some work, it's very
    easy for a user to write code which will hang, or produce an error, and the
    cases that use it properly are far more complicated than ones that don't.
    You might want to consider a different API:
    - Passing a func(string) callback as progress makes it easy to use
    in all cases, and harder to get wrong. It's how filepath.Walk works.
    - Make the whole operation an object with a state that is used in
    a loop (one iteration per file processed), similar to how
    http://golang.org/pkg/bufio/#Scanner works. This provides several
    benefits:
    - Better control for programmers using the code, as they can
    decide themselves when to proceed, and possibly allow them to cancel the
    operation.
    - Easier to add features without breaking backwards
    compatibility (eg: add new methods to query per-iteration)

    Here is an example of how the above can be used to simplify the code, and
    make a simple zip replacement program:
    http://play.golang.org/p/zvc7p2Tqna
    On Friday, September 20, 2013 4:56:38 AM UTC-7, Pierre Durand wrote:

    https://github.com/pierrre/archivefile

    It's just a helper for the "archive" package that allows to (un)archive
    file/directory to/from file/writer/reader.

    Currently, only "archive/zip" is supported.

    It is used in https://github.com/pierrre/mangadownloader for .cbz
    support.
    --
    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.
  • Pierre Durand at Sep 23, 2013 at 6:57 pm
    1/2/3. You're right, my code is now easier to use.
    I actually found it odd :P

    4. I know it's not very useful.
    I don't plan to rewrite winzip in Go :P
    It's just that I didn't want to have this code in "mangadownloader" (for
    modularity)
    I really appreciate your advices :) (btw I have another project
    https://github.com/pierrre/imageserver
    https://groups.google.com/forum/#!topic/golang-nuts/ITxRD9FLOoU )

    5. I'll try to write a better doc, but I don't like verbose docs (and I'm
    not used to express myself in English :( )

    6. Ok
    Testing is weird in Go (no mock/stub), but I'll learn :D
    Is it ok to use examples/test for coverage?

    7. I already knew go vet / race / cover
    I'll have a look to the others

    I'll implement a new version with filepath.Walk()

    About "operation as object", do you mean:
    type Archiver struct{
    }
    func (archiver *Archiver) Archive(path string) {
    }
    func Archive(path string) {
       archiver := new(Archiver)
       archiver.Archive(path)
    }

    Le lundi 23 septembre 2013 16:54:16 UTC+2, Carlos Castillo a écrit :
    I didn't put an example of the "operation as object" in my code. I pointed
    at the bufio.Scanner object as example of that pattern. I just meant by my
    second point that you might want to reconsider the API, and gave two
    alternatives, a simple one for you to try (callbacks), as well as a
    completely different one (resulting in a lot of refactoring) that would
    offer some new benefits.

    My main point was to show that filepath.Walk already does most of what you
    tried to re-create with ioutil.ReadDir and 3 recursive functions, and that
    your method for returning progress could be improved.

    What may have confused you was the code at the top, the Archiver
    interface, which I created to show how you can separate the zip-specific
    functionality from the code. Here is how the code looks without it:
    http://play.golang.org/p/yCshfPjO2W

    Some things to consider for your current and future packages:

    1. Channels are a nifty new feature of Go, but great care should be
    used when putting them into the exported API of a package. In some cases,
    like your old code, they require the user of your package to create
    parallel code to use it properly which complicates their side greatly, and
    can have some unfortunate side-effects if used improperly. You fortunately
    allowed a nil progress variable to be passed in, meaning that the user no
    longer needed concurrent code to run your functions, but it means that the
    code that used the progress feature were far more complicated than the code
    that didn't.
    2. In tandem with above, most packages should not force the user to
    create their own parallel go-routines to effectively use them.
    3. The above points don't prevent you from using goroutines/channels
    inside your package, just that having them as part of the API leaves a
    minefield of potential problems for the users of your package, where users
    have to (in the case of the old archivefile/zip) read the code themselves
    to see how the channel is used to understand what they need to do to
    prevent the code from blocking (run a separate go-routine), and how to end
    their progress checking loop (they have to manually do it themselves).
    4. Try not to take this the wrong way: This package isn't very useful.
    For people looking to pack/unpack a zip-file full of files from/to a
    folder, many tools/programs already exist for that (zip, WinZip, 7-zip,
    context menu options built-in to OSX and Windows, etc...). Those that want
    to do it from within a go program will probably just use archive/zip
    directly themselves, since it has far fewer restrictions than your code,
    which is built for a very specific set of cases. With only a few dozen
    lines, I was able to re-create the functionality of your package that I
    cared about, the big difference being that it's my code so I understood it
    better, and could then modify it to do additional tasks such as filter
    desired folders/files, pre-process others, or do whatever else I wanted.
    This code will probably still be useful to you, and you can keep it around
    should you need to pack a folder of files into a zip-file as part of a
    larger operation, but most people won't use it enough for it to be useful
    to them to keep it around. Most likely, this package's most important uses
    have already happened:
    1. to implement a feature in mangadownloader
    2. to allow others to critique your coding style to help you learn
    to how to better program in go :-)
    5. In general documentation can almost always be better. It can also
    help if you can't avoid some of the problems above. I also use the API and
    documentation shown on godoc.org as a first impression to see if a
    package is even worth considering.
    6. More tests, including ones that check that error cases fail
    properly, benefit a package's stability.
    - Your current test coverage for archivefile/zip is at 61.5%
    (reported by go 1.2's coverage tool: go test -cover)
    - mangadownloader is at 54%
    7. You should be aware of / consider the output of tools that exist to
    assist in creating better code and assist in finding problems:
    - go vet
    - The race detector (go build -race / go test -race)
    - go test -cover (new in Go 1.2) and it's html output option
    - github.com/golang/lint/golint - for coding style tips (run with:
    "golint ." or "golint pkg" NOT "golint")
    - github.com/remyoudompheng/go-misc/deadcode - to find unused &
    unexported code
    - github.com/kisielk/errcheck - to find cases where errors are
    ignored
    - Be careful when running the external tools, since they might
    parse their command lines differently then the built-in tools (eg: golint
    with no args does nothing; it produces no output, and no errors)

    On Monday, September 23, 2013 4:43:33 AM UTC-7, Pierre Durand wrote:

    Thank you!

    I had not realized that filepath.Walk() worked recursively.
    I'll use it.

    A function is easier to use than a channel.
    I'll create my own type, like WalkFunc.

    I don't understand "Make the whole operation an object".
    In you example, you create a new zip writer.
    It defeats the purpose of my library :P (it's harder to use)

    Le lundi 23 septembre 2013 03:55:05 UTC+2, Carlos Castillo a écrit :
    Some suggestions:

    1. As you found out ioutil.ReadDir() only returns the immediate
    contents of the given directory, not the full tree of the given directory.
    If you use filepath.Walk instead you can simplify your code quite a bit.
    2. Your method for displaying progress could use some work, it's
    very easy for a user to write code which will hang, or produce an error,
    and the cases that use it properly are far more complicated than ones that
    don't. You might want to consider a different API:
    - Passing a func(string) callback as progress makes it easy to
    use in all cases, and harder to get wrong. It's how filepath.Walk works.
    - Make the whole operation an object with a state that is used in
    a loop (one iteration per file processed), similar to how
    http://golang.org/pkg/bufio/#Scanner works. This provides several
    benefits:
    - Better control for programmers using the code, as they can
    decide themselves when to proceed, and possibly allow them to cancel the
    operation.
    - Easier to add features without breaking backwards
    compatibility (eg: add new methods to query per-iteration)

    Here is an example of how the above can be used to simplify the code,
    and make a simple zip replacement program:
    http://play.golang.org/p/zvc7p2Tqna
    On Friday, September 20, 2013 4:56:38 AM UTC-7, Pierre Durand wrote:

    https://github.com/pierrre/archivefile

    It's just a helper for the "archive" package that allows to (un)archive
    file/directory to/from file/writer/reader.

    Currently, only "archive/zip" is supported.

    It is used in https://github.com/pierrre/mangadownloader for .cbz
    support.
    --
    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.
  • Pierre Durand at Sep 23, 2013 at 8:09 pm
    What is the purpose of:
    if err != nil || i.IsDir() {
    return err
    }

    in your WalkFunc?


    Le lundi 23 septembre 2013 20:52:26 UTC+2, Pierre Durand a écrit :
    1/2/3. You're right, my code is now easier to use.
    I actually found it odd :P

    4. I know it's not very useful.
    I don't plan to rewrite winzip in Go :P
    It's just that I didn't want to have this code in "mangadownloader" (for
    modularity)
    I really appreciate your advices :) (btw I have another project
    https://github.com/pierrre/imageserver
    https://groups.google.com/forum/#!topic/golang-nuts/ITxRD9FLOoU )

    5. I'll try to write a better doc, but I don't like verbose docs (and I'm
    not used to express myself in English :( )

    6. Ok
    Testing is weird in Go (no mock/stub), but I'll learn :D
    Is it ok to use examples/test for coverage?

    7. I already knew go vet / race / cover
    I'll have a look to the others

    I'll implement a new version with filepath.Walk()

    About "operation as object", do you mean:
    type Archiver struct{
    }
    func (archiver *Archiver) Archive(path string) {
    }
    func Archive(path string) {
    archiver := new(Archiver)
    archiver.Archive(path)
    }

    Le lundi 23 septembre 2013 16:54:16 UTC+2, Carlos Castillo a écrit :
    I didn't put an example of the "operation as object" in my code. I
    pointed at the bufio.Scanner object as example of that pattern. I just
    meant by my second point that you might want to reconsider the API, and
    gave two alternatives, a simple one for you to try (callbacks), as well as
    a completely different one (resulting in a lot of refactoring) that would
    offer some new benefits.

    My main point was to show that filepath.Walk already does most of what
    you tried to re-create with ioutil.ReadDir and 3 recursive functions, and
    that your method for returning progress could be improved.

    What may have confused you was the code at the top, the Archiver
    interface, which I created to show how you can separate the zip-specific
    functionality from the code. Here is how the code looks without it:
    http://play.golang.org/p/yCshfPjO2W

    Some things to consider for your current and future packages:

    1. Channels are a nifty new feature of Go, but great care should be
    used when putting them into the exported API of a package. In some cases,
    like your old code, they require the user of your package to create
    parallel code to use it properly which complicates their side greatly, and
    can have some unfortunate side-effects if used improperly. You fortunately
    allowed a nil progress variable to be passed in, meaning that the user no
    longer needed concurrent code to run your functions, but it means that the
    code that used the progress feature were far more complicated than the code
    that didn't.
    2. In tandem with above, most packages should not force the user to
    create their own parallel go-routines to effectively use them.
    3. The above points don't prevent you from using goroutines/channels
    inside your package, just that having them as part of the API leaves a
    minefield of potential problems for the users of your package, where users
    have to (in the case of the old archivefile/zip) read the code themselves
    to see how the channel is used to understand what they need to do to
    prevent the code from blocking (run a separate go-routine), and how to end
    their progress checking loop (they have to manually do it themselves).
    4. Try not to take this the wrong way: This package isn't very
    useful. For people looking to pack/unpack a zip-file full of files from/to
    a folder, many tools/programs already exist for that (zip, WinZip, 7-zip,
    context menu options built-in to OSX and Windows, etc...). Those that want
    to do it from within a go program will probably just use archive/zip
    directly themselves, since it has far fewer restrictions than your code,
    which is built for a very specific set of cases. With only a few dozen
    lines, I was able to re-create the functionality of your package that I
    cared about, the big difference being that it's my code so I understood it
    better, and could then modify it to do additional tasks such as filter
    desired folders/files, pre-process others, or do whatever else I wanted.
    This code will probably still be useful to you, and you can keep it around
    should you need to pack a folder of files into a zip-file as part of a
    larger operation, but most people won't use it enough for it to be useful
    to them to keep it around. Most likely, this package's most important uses
    have already happened:
    1. to implement a feature in mangadownloader
    2. to allow others to critique your coding style to help you learn
    to how to better program in go :-)
    5. In general documentation can almost always be better. It can also
    help if you can't avoid some of the problems above. I also use the API and
    documentation shown on godoc.org as a first impression to see if a
    package is even worth considering.
    6. More tests, including ones that check that error cases fail
    properly, benefit a package's stability.
    - Your current test coverage for archivefile/zip is at 61.5%
    (reported by go 1.2's coverage tool: go test -cover)
    - mangadownloader is at 54%
    7. You should be aware of / consider the output of tools that exist
    to assist in creating better code and assist in finding problems:
    - go vet
    - The race detector (go build -race / go test -race)
    - go test -cover (new in Go 1.2) and it's html output option
    - github.com/golang/lint/golint - for coding style tips (run with:
    "golint ." or "golint pkg" NOT "golint")
    - github.com/remyoudompheng/go-misc/deadcode - to find unused &
    unexported code
    - github.com/kisielk/errcheck - to find cases where errors are
    ignored
    - Be careful when running the external tools, since they might
    parse their command lines differently then the built-in tools (eg: golint
    with no args does nothing; it produces no output, and no errors)

    On Monday, September 23, 2013 4:43:33 AM UTC-7, Pierre Durand wrote:

    Thank you!

    I had not realized that filepath.Walk() worked recursively.
    I'll use it.

    A function is easier to use than a channel.
    I'll create my own type, like WalkFunc.

    I don't understand "Make the whole operation an object".
    In you example, you create a new zip writer.
    It defeats the purpose of my library :P (it's harder to use)

    Le lundi 23 septembre 2013 03:55:05 UTC+2, Carlos Castillo a écrit :
    Some suggestions:

    1. As you found out ioutil.ReadDir() only returns the immediate
    contents of the given directory, not the full tree of the given directory.
    If you use filepath.Walk instead you can simplify your code quite a bit.
    2. Your method for displaying progress could use some work, it's
    very easy for a user to write code which will hang, or produce an error,
    and the cases that use it properly are far more complicated than ones that
    don't. You might want to consider a different API:
    - Passing a func(string) callback as progress makes it easy to
    use in all cases, and harder to get wrong. It's how filepath.Walk works.
    - Make the whole operation an object with a state that is used
    in a loop (one iteration per file processed), similar to how
    http://golang.org/pkg/bufio/#Scanner works. This provides
    several benefits:
    - Better control for programmers using the code, as they can
    decide themselves when to proceed, and possibly allow them to cancel the
    operation.
    - Easier to add features without breaking backwards
    compatibility (eg: add new methods to query per-iteration)

    Here is an example of how the above can be used to simplify the code,
    and make a simple zip replacement program:
    http://play.golang.org/p/zvc7p2Tqna
    On Friday, September 20, 2013 4:56:38 AM UTC-7, Pierre Durand wrote:

    https://github.com/pierrre/archivefile

    It's just a helper for the "archive" package that allows to
    (un)archive file/directory to/from file/writer/reader.

    Currently, only "archive/zip" is supported.

    It is used in https://github.com/pierrre/mangadownloader for .cbz
    support.
    --
    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 Sep 23, 2013 at 10:40 pm
    Occasionally an error occurs, so err (an argument to walkfunc) is not nil,
    also in the case of zip, we don't need to do any work for directories, so
    that if statement handles all cases:

        1. If err is not nil, and the item was not a dir, then return the error
        2. If err is not nil, and the item was a dir, then return the error
        3. If err is nil, but the item is a dir, then return nil (the value of
        err) to go to next item
        4. If err is nil, and item is not dir, then continue on to rest of
        walkfunc body.

    On Monday, September 23, 2013 1:09:08 PM UTC-7, Pierre Durand wrote:

    What is the purpose of:
    if err != nil || i.IsDir() {
    return err
    }

    in your WalkFunc?


    Le lundi 23 septembre 2013 20:52:26 UTC+2, Pierre Durand a écrit :
    1/2/3. You're right, my code is now easier to use.
    I actually found it odd :P

    4. I know it's not very useful.
    I don't plan to rewrite winzip in Go :P
    It's just that I didn't want to have this code in "mangadownloader" (for
    modularity)
    I really appreciate your advices :) (btw I have another project
    https://github.com/pierrre/imageserver
    https://groups.google.com/forum/#!topic/golang-nuts/ITxRD9FLOoU )

    5. I'll try to write a better doc, but I don't like verbose docs (and I'm
    not used to express myself in English :( )

    6. Ok
    Testing is weird in Go (no mock/stub), but I'll learn :D
    Is it ok to use examples/test for coverage?

    7. I already knew go vet / race / cover
    I'll have a look to the others

    I'll implement a new version with filepath.Walk()

    About "operation as object", do you mean:
    type Archiver struct{
    }
    func (archiver *Archiver) Archive(path string) {
    }
    func Archive(path string) {
    archiver := new(Archiver)
    archiver.Archive(path)
    }

    Le lundi 23 septembre 2013 16:54:16 UTC+2, Carlos Castillo a écrit :
    I didn't put an example of the "operation as object" in my code. I
    pointed at the bufio.Scanner object as example of that pattern. I just
    meant by my second point that you might want to reconsider the API, and
    gave two alternatives, a simple one for you to try (callbacks), as well as
    a completely different one (resulting in a lot of refactoring) that would
    offer some new benefits.

    My main point was to show that filepath.Walk already does most of what
    you tried to re-create with ioutil.ReadDir and 3 recursive functions, and
    that your method for returning progress could be improved.

    What may have confused you was the code at the top, the Archiver
    interface, which I created to show how you can separate the zip-specific
    functionality from the code. Here is how the code looks without it:
    http://play.golang.org/p/yCshfPjO2W

    Some things to consider for your current and future packages:

    1. Channels are a nifty new feature of Go, but great care should be
    used when putting them into the exported API of a package. In some cases,
    like your old code, they require the user of your package to create
    parallel code to use it properly which complicates their side greatly, and
    can have some unfortunate side-effects if used improperly. You fortunately
    allowed a nil progress variable to be passed in, meaning that the user no
    longer needed concurrent code to run your functions, but it means that the
    code that used the progress feature were far more complicated than the code
    that didn't.
    2. In tandem with above, most packages should not force the user to
    create their own parallel go-routines to effectively use them.
    3. The above points don't prevent you from using goroutines/channels
    inside your package, just that having them as part of the API leaves a
    minefield of potential problems for the users of your package, where users
    have to (in the case of the old archivefile/zip) read the code themselves
    to see how the channel is used to understand what they need to do to
    prevent the code from blocking (run a separate go-routine), and how to end
    their progress checking loop (they have to manually do it themselves).
    4. Try not to take this the wrong way: This package isn't very
    useful. For people looking to pack/unpack a zip-file full of files from/to
    a folder, many tools/programs already exist for that (zip, WinZip, 7-zip,
    context menu options built-in to OSX and Windows, etc...). Those that want
    to do it from within a go program will probably just use archive/zip
    directly themselves, since it has far fewer restrictions than your code,
    which is built for a very specific set of cases. With only a few dozen
    lines, I was able to re-create the functionality of your package that I
    cared about, the big difference being that it's my code so I understood it
    better, and could then modify it to do additional tasks such as filter
    desired folders/files, pre-process others, or do whatever else I wanted.
    This code will probably still be useful to you, and you can keep it around
    should you need to pack a folder of files into a zip-file as part of a
    larger operation, but most people won't use it enough for it to be useful
    to them to keep it around. Most likely, this package's most important uses
    have already happened:
    1. to implement a feature in mangadownloader
    2. to allow others to critique your coding style to help you
    learn to how to better program in go :-)
    5. In general documentation can almost always be better. It can also
    help if you can't avoid some of the problems above. I also use the API and
    documentation shown on godoc.org as a first impression to see if a
    package is even worth considering.
    6. More tests, including ones that check that error cases fail
    properly, benefit a package's stability.
    - Your current test coverage for archivefile/zip is at 61.5%
    (reported by go 1.2's coverage tool: go test -cover)
    - mangadownloader is at 54%
    7. You should be aware of / consider the output of tools that exist
    to assist in creating better code and assist in finding problems:
    - go vet
    - The race detector (go build -race / go test -race)
    - go test -cover (new in Go 1.2) and it's html output option
    - github.com/golang/lint/golint - for coding style tips (run
    with: "golint ." or "golint pkg" NOT "golint")
    - github.com/remyoudompheng/go-misc/deadcode - to find unused &
    unexported code
    - github.com/kisielk/errcheck - to find cases where errors are
    ignored
    - Be careful when running the external tools, since they might
    parse their command lines differently then the built-in tools (eg: golint
    with no args does nothing; it produces no output, and no errors)

    On Monday, September 23, 2013 4:43:33 AM UTC-7, Pierre Durand wrote:

    Thank you!

    I had not realized that filepath.Walk() worked recursively.
    I'll use it.

    A function is easier to use than a channel.
    I'll create my own type, like WalkFunc.

    I don't understand "Make the whole operation an object".
    In you example, you create a new zip writer.
    It defeats the purpose of my library :P (it's harder to use)

    Le lundi 23 septembre 2013 03:55:05 UTC+2, Carlos Castillo a écrit :
    Some suggestions:

    1. As you found out ioutil.ReadDir() only returns the immediate
    contents of the given directory, not the full tree of the given directory.
    If you use filepath.Walk instead you can simplify your code quite a bit.
    2. Your method for displaying progress could use some work, it's
    very easy for a user to write code which will hang, or produce an error,
    and the cases that use it properly are far more complicated than ones that
    don't. You might want to consider a different API:
    - Passing a func(string) callback as progress makes it easy to
    use in all cases, and harder to get wrong. It's how filepath.Walk works.
    - Make the whole operation an object with a state that is used
    in a loop (one iteration per file processed), similar to how
    http://golang.org/pkg/bufio/#Scanner works. This provides
    several benefits:
    - Better control for programmers using the code, as they can
    decide themselves when to proceed, and possibly allow them to cancel the
    operation.
    - Easier to add features without breaking backwards
    compatibility (eg: add new methods to query per-iteration)

    Here is an example of how the above can be used to simplify the code,
    and make a simple zip replacement program:
    http://play.golang.org/p/zvc7p2Tqna
    On Friday, September 20, 2013 4:56:38 AM UTC-7, Pierre Durand wrote:

    https://github.com/pierrre/archivefile

    It's just a helper for the "archive" package that allows to
    (un)archive file/directory to/from file/writer/reader.

    Currently, only "archive/zip" is supported.

    It is used in https://github.com/pierrre/mangadownloader for .cbz
    support.
    --
    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.
  • Pierre Durand at Sep 24, 2013 at 8:21 am
    Ok!
    "3." misled me.

    Le mardi 24 septembre 2013 00:40:38 UTC+2, Carlos Castillo a écrit :
    Occasionally an error occurs, so err (an argument to walkfunc) is not nil,
    also in the case of zip, we don't need to do any work for directories, so
    that if statement handles all cases:

    1. If err is not nil, and the item was not a dir, then return the error
    2. If err is not nil, and the item was a dir, then return the error
    3. If err is nil, but the item is a dir, then return nil (the value of
    err) to go to next item
    4. If err is nil, and item is not dir, then continue on to rest of
    walkfunc body.

    On Monday, September 23, 2013 1:09:08 PM UTC-7, Pierre Durand wrote:

    What is the purpose of:
    if err != nil || i.IsDir() {
    return err
    }

    in your WalkFunc?


    Le lundi 23 septembre 2013 20:52:26 UTC+2, Pierre Durand a écrit :
    1/2/3. You're right, my code is now easier to use.
    I actually found it odd :P

    4. I know it's not very useful.
    I don't plan to rewrite winzip in Go :P
    It's just that I didn't want to have this code in "mangadownloader" (for
    modularity)
    I really appreciate your advices :) (btw I have another project
    https://github.com/pierrre/imageserver
    https://groups.google.com/forum/#!topic/golang-nuts/ITxRD9FLOoU )

    5. I'll try to write a better doc, but I don't like verbose docs (and
    I'm not used to express myself in English :( )

    6. Ok
    Testing is weird in Go (no mock/stub), but I'll learn :D
    Is it ok to use examples/test for coverage?

    7. I already knew go vet / race / cover
    I'll have a look to the others

    I'll implement a new version with filepath.Walk()

    About "operation as object", do you mean:
    type Archiver struct{
    }
    func (archiver *Archiver) Archive(path string) {
    }
    func Archive(path string) {
    archiver := new(Archiver)
    archiver.Archive(path)
    }

    Le lundi 23 septembre 2013 16:54:16 UTC+2, Carlos Castillo a écrit :
    I didn't put an example of the "operation as object" in my code. I
    pointed at the bufio.Scanner object as example of that pattern. I just
    meant by my second point that you might want to reconsider the API, and
    gave two alternatives, a simple one for you to try (callbacks), as well as
    a completely different one (resulting in a lot of refactoring) that would
    offer some new benefits.

    My main point was to show that filepath.Walk already does most of what
    you tried to re-create with ioutil.ReadDir and 3 recursive functions, and
    that your method for returning progress could be improved.

    What may have confused you was the code at the top, the Archiver
    interface, which I created to show how you can separate the zip-specific
    functionality from the code. Here is how the code looks without it:
    http://play.golang.org/p/yCshfPjO2W

    Some things to consider for your current and future packages:

    1. Channels are a nifty new feature of Go, but great care should be
    used when putting them into the exported API of a package. In some cases,
    like your old code, they require the user of your package to create
    parallel code to use it properly which complicates their side greatly, and
    can have some unfortunate side-effects if used improperly. You fortunately
    allowed a nil progress variable to be passed in, meaning that the user no
    longer needed concurrent code to run your functions, but it means that the
    code that used the progress feature were far more complicated than the code
    that didn't.
    2. In tandem with above, most packages should not force the user to
    create their own parallel go-routines to effectively use them.
    3. The above points don't prevent you from using
    goroutines/channels inside your package, just that having them as part of
    the API leaves a minefield of potential problems for the users of your
    package, where users have to (in the case of the old archivefile/zip) read
    the code themselves to see how the channel is used to understand what they
    need to do to prevent the code from blocking (run a separate go-routine),
    and how to end their progress checking loop (they have to manually do it
    themselves).
    4. Try not to take this the wrong way: This package isn't very
    useful. For people looking to pack/unpack a zip-file full of files from/to
    a folder, many tools/programs already exist for that (zip, WinZip, 7-zip,
    context menu options built-in to OSX and Windows, etc...). Those that want
    to do it from within a go program will probably just use archive/zip
    directly themselves, since it has far fewer restrictions than your code,
    which is built for a very specific set of cases. With only a few dozen
    lines, I was able to re-create the functionality of your package that I
    cared about, the big difference being that it's my code so I understood it
    better, and could then modify it to do additional tasks such as filter
    desired folders/files, pre-process others, or do whatever else I wanted.
    This code will probably still be useful to you, and you can keep it around
    should you need to pack a folder of files into a zip-file as part of a
    larger operation, but most people won't use it enough for it to be useful
    to them to keep it around. Most likely, this package's most important uses
    have already happened:
    1. to implement a feature in mangadownloader
    2. to allow others to critique your coding style to help you
    learn to how to better program in go :-)
    5. In general documentation can almost always be better. It can
    also help if you can't avoid some of the problems above. I also use the API
    and documentation shown on godoc.org as a first impression to see
    if a package is even worth considering.
    6. More tests, including ones that check that error cases fail
    properly, benefit a package's stability.
    - Your current test coverage for archivefile/zip is at 61.5%
    (reported by go 1.2's coverage tool: go test -cover)
    - mangadownloader is at 54%
    7. You should be aware of / consider the output of tools that exist
    to assist in creating better code and assist in finding problems:
    - go vet
    - The race detector (go build -race / go test -race)
    - go test -cover (new in Go 1.2) and it's html output option
    - github.com/golang/lint/golint - for coding style tips (run
    with: "golint ." or "golint pkg" NOT "golint")
    - github.com/remyoudompheng/go-misc/deadcode - to find unused &
    unexported code
    - github.com/kisielk/errcheck - to find cases where errors are
    ignored
    - Be careful when running the external tools, since they might
    parse their command lines differently then the built-in tools (eg: golint
    with no args does nothing; it produces no output, and no errors)

    On Monday, September 23, 2013 4:43:33 AM UTC-7, Pierre Durand wrote:

    Thank you!

    I had not realized that filepath.Walk() worked recursively.
    I'll use it.

    A function is easier to use than a channel.
    I'll create my own type, like WalkFunc.

    I don't understand "Make the whole operation an object".
    In you example, you create a new zip writer.
    It defeats the purpose of my library :P (it's harder to use)

    Le lundi 23 septembre 2013 03:55:05 UTC+2, Carlos Castillo a écrit :
    Some suggestions:

    1. As you found out ioutil.ReadDir() only returns the immediate
    contents of the given directory, not the full tree of the given directory.
    If you use filepath.Walk instead you can simplify your code quite a bit.
    2. Your method for displaying progress could use some work, it's
    very easy for a user to write code which will hang, or produce an error,
    and the cases that use it properly are far more complicated than ones that
    don't. You might want to consider a different API:
    - Passing a func(string) callback as progress makes it easy to
    use in all cases, and harder to get wrong. It's how filepath.Walk works.
    - Make the whole operation an object with a state that is used
    in a loop (one iteration per file processed), similar to how
    http://golang.org/pkg/bufio/#Scanner works. This provides
    several benefits:
    - Better control for programmers using the code, as they
    can decide themselves when to proceed, and possibly allow them to cancel
    the operation.
    - Easier to add features without breaking backwards
    compatibility (eg: add new methods to query per-iteration)

    Here is an example of how the above can be used to simplify the code,
    and make a simple zip replacement program:
    http://play.golang.org/p/zvc7p2Tqna
    On Friday, September 20, 2013 4:56:38 AM UTC-7, Pierre Durand wrote:

    https://github.com/pierrre/archivefile

    It's just a helper for the "archive" package that allows to
    (un)archive file/directory to/from file/writer/reader.

    Currently, only "archive/zip" is supported.

    It is used in https://github.com/pierrre/mangadownloader for .cbz
    support.
    --
    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
postedSep 20, '13 at 11:56a
activeSep 24, '13 at 8:21a
posts8
users2
websitegolang.org

2 users in discussion

Pierre Durand: 5 posts Carlos Castillo: 3 posts

People

Translate

site design / logo © 2022 Grokbase