Adding functionality to specify a PageSize to AllPages #126
Conversation
| @@ -12,13 +12,23 @@ class AllPagesExpression : Expression | |||
| /// Initializes a new instance of the <see cref="AllPagesExpression"/> class. | |||
| /// </summary> | |||
| /// <param name="method">The method that AllPages() was called on.</param> | |||
| public AllPagesExpression(MethodCallExpression method) => Method = method; | |||
| /// <param name="constant">The ConstantExpression that AllPages was sent</param> | |||
| public AllPagesExpression(MethodCallExpression method, ConstantExpression constant = null) | |||
There was a problem hiding this comment.
Why is this called constant rather than something more descriptive like pageSize?
There was a problem hiding this comment.
I might've been overthinking things...
| @@ -736,6 +738,10 @@ private Expression RewritePagingConnectionExtensions(MethodCallExpression expres | |||
| // is visited. | |||
| return new AllPagesExpression((MethodCallExpression)expression.Arguments[0]); | |||
| } | |||
| else if (expression.Method.GetGenericMethodDefinition() == PagingConnectionExtensions.AllPagesCustomSizeMethod) | |||
| { | |||
| return new AllPagesExpression((MethodCallExpression)expression.Arguments[0], (ConstantExpression)expression.Arguments[1]); | |||
There was a problem hiding this comment.
What if the value passed into AllPages isn't a ConstantExpression? We should at least throw a helpful exception rather than an InvalidCastException here if we don't support it. Would it be difficult to support non-constant expressions?
| @@ -787,16 +793,16 @@ private void AddIdSelection(ISelectionSet set) | |||
| } | |||
| } | |||
|
|
|||
| private ISubquery AddSubquery( | |||
| MethodCallExpression expression, | |||
| private ISubquery AddSubquery(MethodCallExpression expression, | |||
There was a problem hiding this comment.
Why change the formatting here?
There was a problem hiding this comment.
Resharper is greedy sometimes...
| private Expression CreateNodeQuery( | ||
| MethodCallExpression expression, | ||
| MethodCallExpression selector) | ||
| private Expression CreateNodeQuery(MethodCallExpression expression, |
| MethodCallExpression RewritePagingMethodCall( | ||
| MethodCallExpression methodCall, | ||
| Expression instance) | ||
| MethodCallExpression RewritePagingMethodCall(MethodCallExpression methodCall, |
| @@ -52,8 +52,8 @@ public void Viewer_By_GraphyQL_Matches_Api() | |||
| Assert.NotNull(graphqlUser); | |||
|
|
|||
| Assert.Equal(apiUser.AvatarUrl, graphqlUser.AvatarUrl); | |||
| Assert.Equal(apiUser.Bio ?? string.Empty, graphqlUser.Bio); | |||
| Assert.Equal(apiUser.Company ?? string.Empty, graphqlUser.Company); | |||
| Assert.Equal(apiUser.Bio, graphqlUser.Bio); | |||
There was a problem hiding this comment.
This change is from #120, but as I mentioned there, it's causing the test to fail.
There was a problem hiding this comment.
I'll rebase it out.
StanleyGoldman
force-pushed
the
fixes/all-pages-custom-size
branch
from
bf2e3f3
to
8caf8f1
Compare
| @@ -16,13 +16,6 @@ public class PagingTests | |||
|
|
|||
| public class Repository_Issues_AllPages | |||
| { | |||
| ICompiledQuery<IEnumerable<int>> TestQuery { get; } = new Query() | |||
| .Repository("foo", "bar") | |||
There was a problem hiding this comment.
This query being a field caused it to run on every test in this fixture.
There was a problem hiding this comment.
Well to split hairs it wasn't a field, it was a property ;) But yeah it was an auto-property so it was backed by a field.
| .Compile(); | ||
| } | ||
|
|
||
| private static ICompiledQuery<IEnumerable<int>> GetTestQueryCustomSize() |
There was a problem hiding this comment.
Why is this method static when GetTestQuery isn't?
There was a problem hiding this comment.
oversights..
| @@ -16,13 +16,6 @@ public class PagingTests | |||
|
|
|||
| public class Repository_Issues_AllPages | |||
| { | |||
| ICompiledQuery<IEnumerable<int>> TestQuery { get; } = new Query() | |||
| .Repository("foo", "bar") | |||
There was a problem hiding this comment.
Well to split hairs it wasn't a field, it was a property ;) But yeah it was an auto-property so it was backed by a field.
AllPagesThis is integral when dealing with very large queries. Queries are limited by the amount of objects they can possibly return. Not by how many objects they actually return.
Using
AllPages(int)will allow the user to change the$firstvalue and reduce the possible combination in order to craft an operational query.