-
Notifications
You must be signed in to change notification settings - Fork 4
feat!: support dataProperty with ParallelRequestExecutor #305
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
base: main
Are you sure you want to change the base?
Conversation
42d0244
to
9d37255
Compare
This feels way too easy but there seems to be only one test that actually validates Already open for review but I want to have a bit more test coverage. Edit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #305 +/- ##
==========================================
+ Coverage 76.20% 77.32% +1.12%
==========================================
Files 155 152 -3
Lines 5047 4843 -204
Branches 896 843 -53
==========================================
- Hits 3846 3745 -101
+ Misses 861 781 -80
+ Partials 340 317 -23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Removing the one request executor that already works with kotlinx.serialization seems bad for #224 but my gut feeling is that this would still be a step in the right direction. Both executors have diverged and I believe it is easier to migrate the existing one than solving existing issues with |
9d37255
to
7b2d851
Compare
Back with some more test coverage and an implementation that looks a bit more promising, albeit not very polished. |
That might be an issue. Just sharing it here for now as I'm not sure if I am able to analyze further before my absence:
"withNewExecutor" either selects the |
Better. Just needs a bit more polishing than before:
|
0c61053
to
be38453
Compare
Adds proper handling for `dataProperties` to `ParallelRequestExecutor` and gets rid of `DataLoaderPreparedRequestExecutor`, along with the option to configure the request executor in `SchemaConfiguration` and related `DataLoaderPreparedRequestExecutor`-specific options. This also consolidates some tests that were hardcoded to use one of the request executors, and therefore missed the other, and extends coverage for dataProperties a bit. Further, this reduces visibility of some functions to `internal` to reduce namespace pollution. Resolves #58 Resolves #268 BREAKING CHANGE: The option to configure request executors has been removed, and execution will now always use the `ParallelRequestExecutor`. BREAKING CHANGE: The option to configure a timeout has been removed, as it was only respected by `DataLoaderPreparedRequestExecutor`. BREAKING CHANGE: The possibility to pass `ExecutionOptions` to schema execution has been removed, as it was only respected by `DataLoaderPreparedRequestExecutor`. BREAKING CHANGE: The `DataLoaderPreparedRequestExecutor`, `Executor`, and `ExecutionOptions` classes have been removed.
be38453
to
0d499f8
Compare
Adds proper handling for
dataProperties
toParallelRequestExecutor
and gets rid ofDataLoaderPreparedRequestExecutor
, along with the option to configure the request executor inSchemaConfiguration
and relatedDataLoaderPreparedRequestExecutor
-specific options.This also consolidates some tests that were hardcoded to use one of the request executors, and therefore missed the other, and extends coverage for dataProperties a bit. Further, this reduces visibility of some functions to
internal
to reduce namespace pollution.Resolves #58
Resolves #268
BREAKING CHANGE: The option to configure request executors has been removed, and execution will now always use the
ParallelRequestExecutor
.BREAKING CHANGE: The option to configure a timeout has been removed, as it was only respected by
DataLoaderPreparedRequestExecutor
.BREAKING CHANGE: The possibility to pass
ExecutionOptions
to schema execution has been removed, as it was only respected byDataLoaderPreparedRequestExecutor
.BREAKING CHANGE: The
DataLoaderPreparedRequestExecutor
,Executor
, andExecutionOptions
classes have been removed.