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

feature: result formatter extensibility with minor refactor and sever… #58

Merged
merged 1 commit into from
Feb 24, 2017

Conversation

dsalahutdinov
Copy link
Contributor

…al testsy

Hi. Some refactor to make adding new result view format depend of content-type easy

@asciimoo
Copy link
Owner

@dsalahutdinov great pull request, thanks. #57 consolidates lots of things in the code, so it would be good to merge that before this PR, if you agree.

@dsalahutdinov
Copy link
Contributor Author

dsalahutdinov commented Feb 16, 2017

@asciimoo , that's ok, i will renew my PR after merging of those

@asciimoo
Copy link
Owner

@dsalahutdinov thanks.
#59 (reopened #57) merged, please rebase.

@dsalahutdinov
Copy link
Contributor Author

@asciimoo i've done
Please take a look at .travis.yml, it has been set to run tests instead of build binaries

.travis.yml Outdated
@@ -9,9 +9,7 @@ os:
env:
- "PATH=/home/travis/gopath/bin:$PATH"
script:
- test -z "$(gofmt -l ./)"
- test -z "$(go vet -v ./...)"
- go build
Copy link
Owner

@asciimoo asciimoo Feb 16, 2017

Choose a reason for hiding this comment

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

I think, these checks would be still useful. Does go test evaluate these too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, unless go test already does it, the gofmt check should definitely remain there. It's already caught a couple misformatted PRs.

Copy link
Contributor

@Benaiah Benaiah left a comment

Choose a reason for hiding this comment

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

This looks great to me. I don't know if we should remove the previous Travis checks, but the go test line you added is good.

@asciimoo
Copy link
Owner

+1, except for travis mods. @dsalahutdinov could you please add go fmt to travis and rebase it to the current master?

@dsalahutdinov
Copy link
Contributor Author

Yes. I'll do it soon. Having some holidays now

@dsalahutdinov
Copy link
Contributor Author

@asciimoo , i've done

Copy link
Owner

@asciimoo asciimoo left a comment

Choose a reason for hiding this comment

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

Awesome PR. Please fix that one minor note. Thanks

wuzz.go Outdated
}
}
responseFormatter := formatter.New(a.config, req.ContentType)
vrb.Title = "Response body (F9) " + responseFormatter.Title()
Copy link
Owner

Choose a reason for hiding this comment

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

Could you use https://github.com/asciimoo/wuzz/blob/master/wuzz.go#L191 instead of hard-coding the title prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, sorry for my inattention 😞

@asciimoo
Copy link
Owner

Thank you

@asciimoo asciimoo merged commit 5a15ac3 into asciimoo:master Feb 24, 2017
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.

3 participants