Skip to content
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

Add support for DateTime in queries #226

Closed
wants to merge 1 commit into from

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Apr 21, 2020

The QuerySerializer would generate a stack overflow without a special case for the DateTimeOffset type.

Note: the DateTime scalar type is mapped to System.DateTimeOffset in the model generator.

The motivation behind this change is that you need to set from and to in order to get more than one year of contributions.

@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #226 (2711727) into master (332e997) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 2711727 differs from pull request most recent head 3da1ebe. Consider uploading reports for the commit 3da1ebe to get more accurate results

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage   78.83%   78.84%   +0.01%     
==========================================
  Files          73       73              
  Lines        2447     2449       +2     
  Branches      357      358       +1     
==========================================
+ Hits         1929     1931       +2     
  Misses        441      441              
  Partials       77       77              
Impacted Files Coverage Δ
...kit.GraphQL.Core/Core/Syntax/VariableDefinition.cs 78.94% <100.00%> (+2.47%) ⬆️

@0xced
Copy link
Contributor Author

0xced commented Apr 21, 2020

And right after creating the pull request, I notice that #214 and #222 are addressing the exact same issues as this pull request. 🤦🏻‍♂️

@brphelps
Copy link
Collaborator

Sorry @0xced -- I was running into a couple different issues while merging. This is now completed

@0xced
Copy link
Contributor Author

0xced commented Apr 28, 2020

No problem, thanks for merging #214! Please don't forget to also merge #222 which is needed to work with DateTime variables.

@jcansdale
Copy link
Collaborator

Hi @0xced 👋

I notice that #222 hasn't been merged yet. I was wondering if you might be up for reviewing it? 🙏

Note: the [DateTime][1] scalar type is [mapped to System.DateTimeOffset][2] in the model generator.

The motivation behind this change is that you need to set `from` and `to` in order to [get more than one year of contributions][3].

[1]: https://developer.github.com/v4/scalar/datetime/
[2]: https://github.com/octokit/octokit.graphql.net/blob/285fc1b2ce0b2dde72a4fa6abb5154778bbbe86d/Octokit.GraphQL.Core.Generation/Utilities/TypeUtilities.cs#L131
[3]: https://git.luolix.topmunity/t5/GitHub-API-Development-and/Are-pullRequestContributions-limited/m-p/32096#M3044
@nickfloyd
Copy link
Contributor

Closing this in favor of: #222

@nickfloyd nickfloyd closed this Aug 1, 2022
@0xced 0xced deleted the query-datetime-support branch August 1, 2022 15:37
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.

4 participants