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

Adds fixes for fetch URL PR #1

Merged
merged 1 commit into from
Jun 5, 2016

Conversation

tomnomnom
Copy link

A great start!

Here's a few suggestions/tweaks to your PR (so this is a PR for your PR - yo dawg).

  1. Updated the help and readme to reflect the changes
  2. Added a new exit code so that failing to fetch a URL is distinct from failing to read a file
  3. Renames url_tests.go to url_test.go; that's what go test expects, your tests weren't actually being run ;)
  4. Refactored the 4 test methods into a single table-driven test.

Table-driven tests are super useful when you want to test a bunch of inputs to a single function, and massively reduce the scope for typo or copy+paste type errors.

You definitely did the bulk of the work here though, so thanks again!

Also, nice one for using 'gron' as a verb; I think I might using 'gronning' as the term for transforming an object to discrete assignments from now on :-D

If you're happy with my changes then merge them and I'll accept your original PR.

Thanks!

@iamthemovie
Copy link
Owner

Thanks Tom.

Ahh you know what happened with those tests? I had my editor set to use spaces not tabs and gofmt complained (as it would) so I ran gofmt and re-outputted the file to the wrong file name facepalm.

"gron" just rolls off the tongue really!

I'll merge this in now.

@iamthemovie iamthemovie merged commit 0a7fd94 into iamthemovie:master Jun 5, 2016
@tomnomnom tomnomnom deleted the fetchurl-fixes branch June 5, 2016 20:42
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