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

contrib/emicklei/go-restful: add go-restful integration #331

Merged
merged 1 commit into from
Sep 24, 2018

Conversation

anggorodewanto
Copy link
Contributor

No description provided.

@gbbr gbbr added the apm:ecosystem contrib/* related feature requests or bugs label Sep 13, 2018
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this! It's looking great. I'm happy to merge it after addressing these minor style related comments.

contrib/emicklei/go-restful/restful.go Outdated Show resolved Hide resolved
contrib/emicklei/go-restful/restful.go Outdated Show resolved Hide resolved
contrib/emicklei/go-restful/restful_test.go Outdated Show resolved Hide resolved
contrib/emicklei/go-restful/restful_test.go Outdated Show resolved Hide resolved
@gbbr gbbr modified the milestones: next, 1.4.0 Sep 19, 2018
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes and for your patience. Just a couple more nits.

contrib/emicklei/go-restful/restful_test.go Outdated Show resolved Hide resolved
contrib/emicklei/go-restful/restful_test.go Outdated Show resolved Hide resolved
contrib/emicklei/go-restful/restful_test.go Outdated Show resolved Hide resolved
contrib/emicklei/go-restful/restful_test.go Outdated Show resolved Hide resolved
@gbbr
Copy link
Contributor

gbbr commented Sep 20, 2018

If you could allow me to edit your PR, I could take a look at that test and see if we can merge them into one single test. Here's a link on how to do that: https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

@anggorodewanto
Copy link
Contributor Author

I already allowed edits from maintainer, so feel free to modify it as you see fit

@gbbr
Copy link
Contributor

gbbr commented Sep 20, 2018

Thanks @anggorodewanto! I'm happy enough if you address all comments except the one with the sub-tests. I don't feel so strongly about it.

@anggorodewanto anggorodewanto force-pushed the go-restful-contrib branch 2 times, most recently from 38a73e4 to cb56e7b Compare September 20, 2018 15:23
@anggorodewanto
Copy link
Contributor Author

No prob! I've addressed them and squashed the commits

@gbbr
Copy link
Contributor

gbbr commented Sep 20, 2018

Thanks a lot! I will review again soon and it should be good to go!

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

One last thing to improve on the example.

contrib/emicklei/go-restful/example_test.go Outdated Show resolved Hide resolved
@anggorodewanto
Copy link
Contributor Author

Updated the example and squashed the commits

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Well done! Thanks for your patience.

For future reference, it might be best not to squash the commits until the review is complete because it makes it easier to see changes made after addressing some comments.

GitHub also has the "Squash & Merge" UI feature which will do it for you automatically before merging.

Thanks again for your work. Are you using this yourself?

@gbbr gbbr merged commit 5fdaa5b into DataDog:v1 Sep 24, 2018
@anggorodewanto
Copy link
Contributor Author

Sorry about that!

Yeah we just signed the contract with Datadog last week

mingrammer pushed a commit to mingrammer/dd-trace-go that referenced this pull request Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants