Skip to content
This repository was archived by the owner on Mar 7, 2019. It is now read-only.

Conversation

@prashantv
Copy link
Contributor

To avoid issues like #20, we can use url.URL to create the final URL.

r @sandlerben

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great idea. Is it typical to write test cases which call other test cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, table tests are more common, but parameterized tests are also common, e.g.
https://github.com/golang/go/blob/master/src/unicode/utf8/utf8_test.go#L222

In general, we should avoid code duplication, as long as the test is still readable and obvious.

@sandlerben
Copy link
Contributor

@prashantv The coverage went down just because we added one more

if err != nil { 
    log.Fatal(err)
} 

It's frustrating that we can't test that log.Fatal happens. Do you think it would be worthwhile to wrap goTorchCommand inside a function like commandAction so that goTorchCommand would return errors and we would log.Fatal inside commandAction?

@prashantv
Copy link
Contributor Author

@sandlerben re: coverage, I think it's worth considering moving away from the cli package. It overcomplicates the design and makes it harder to write code that just returns err, where you only have a log.Fatal once.

cli is great when you have a complex cli with multiple commands that have different flags etc. However, go-torch only has a single command, so I think you could write a vastly simplified CLI using:
https://github.com/jessevdk/go-flags

Your main function would parse the options and pass an options struct, and you would have a function like

func run(opts *Options) error {

}

It would be strongly typed to pass arguments in tests, the function is a simple interface that returns errors that can be tested, and I think the overall amount of code would be less.

This is a much bigger change, but it might be worth trying out.

@sandlerben
Copy link
Contributor

@prashantv Interesting proposal. I'll look into that when I have some time.

sandlerben added a commit that referenced this pull request Jul 29, 2015
Use url.URL to create the pprof URL
@sandlerben sandlerben merged commit 37a3b31 into master Jul 29, 2015
@sandlerben sandlerben deleted the path branch July 29, 2015 17:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants