Uh oh!
There was an error while loading. Please reload this page.
Refactor server_dap to use command-registration style - #12
Conversation
bf0a0b8 to
c1ce4bcCompare| args = req.dig('arguments') | ||
| case req['command'] | ||
| class RequestHandler |
There was a problem hiding this comment.
Using a class to hold only a single field may seem unnecessary, but this could be an extension point for analytics, etc.
There was a problem hiding this comment.
How do you envision us adding metrics through this class?
There was a problem hiding this comment.
How do you envision us adding metrics through this class?
One approach could be to add a command line option, e,g. --request-handler="Shopify::RequestHandler", allowing a custom class to provided.
Maybe @st0012 already has something else in mind though.
There was a problem hiding this comment.
We may not even need this class at all. We can override register_request
defself.register_request *names, &binstrumented=->{measure_stuffdob.callend}names.each{ |name| REGISTERED_REQUESTS[name]=instrumented}endThere was a problem hiding this comment.
Yeah that would also work, but I worry it may not be resilient to future changes within debug.
There was a problem hiding this comment.
I feel if we don't have a concrete use case for it now, it doesn't make sense for the upstream to accept it. So maybe let's hold back on adding this new class and aim at just patching register_request if we need to?
There was a problem hiding this comment.
I'll draft a PR description and share it in internally for review.
51764da to
8a9b8ddCompare8a9b8dd to
70c0d61Compare| end | ||
| end | ||
| def self.register_request *names, &b |
There was a problem hiding this comment.
The style/naming is not what I'd typically use, but I've tried keep it consistent with what's already in use.
andyw8
commented
Mar 17, 2023
Opened upstream: ruby#931 |
Best viewed with whitespace ignored: https://github.com/Shopify/debug/pull/12/files?w=1
Description
Closeshttps://github.com/Shopify/ruby-dev-exp-issues/issues/840