Conversation
|
Could you please isolate the bug fix from the code reorganization? The bug fix is an easy include, but I doubt I want to include the code reorganization. Whenever possible, I prefer a single method because I can read it from top-to-bottom and know what's going on. When the code is divided into methods, then the reader (me?) has to jump around to find out what's really going on. From a maintenance perspective, I prefer keeping API surface area as small as possible: private instead of public whenever possible, inline instead of extracted whenever possible, local variable instead of instance variable whenever possible, and so on. |
22971c2 to
60f2067
Compare
|
@rmosolgo Thank you, that makes sense. |
60f2067 to
bb0b120
Compare
| assert_graphql_equal(expected_result, result) | ||
| end | ||
|
|
||
| it "doesn't use the query trace return value as the result" do |
There was a problem hiding this comment.
I copied this test into master and found that it passed. Was the fix for it pulled into another branch? (Or if not, what motivated adding this test?)
|
I grabbed the tests from this branch in #5737 and confirmed they pass on master. Since this is just a style change, I'd rather not merge it. If you find a performance or behavior reason to propose any of these changes again, I'll definitely reconsider them 👍 |
GraphQL::Execution::Runner#executeand#begin_executecurrently handle multiplex analysis, query setup, step execution, result finalization, root authorization, operation directives, and root-type dispatch inline.This PR reorganizes those responsibilities into private methods:
SelectionsStepconstruction.The existing result ordering, tracing boundaries, directive finalization, mutation isolation, and stack-error handling are preserved.