Skip to content

Conversation

@janssk1
Copy link

@janssk1 janssk1 commented Feb 14, 2020

No description provided.

@brian-brazil
Copy link
Contributor

This change is bigger than it needs to be, you only needed to change the visibility.

@janssk1
Copy link
Author

janssk1 commented Feb 14, 2020

Created it as a toplevel class. That makes it clear that the handler is a standalone class. Instantiating HttpServer.HTTPMetricHandler might indicate that a server is still involved.
Given that there are good tests, moving a class should not be problem.

@brian-brazil
Copy link
Contributor

I'm not sure how good our tests are here, and I'd don't like splitting logic like this across files too much.

@brian-brazil
Copy link
Contributor

Given that my comments remain unaddressed I'm going to close this off. If you're still interesting in a change that only makes this public, let me know.

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