Skip to content

Conversation

MichaelDenwood
Copy link
Contributor

#243
Added a new overload of CreateMethodCall that accepts Expression<Func<TObject, IEnumerable>> selector, means that
Implemented ToList Method here : octokit.graphql.net/Octokit.GraphQL.Core/QueryableListExtensions.cs

MichaelDenwood and others added 3 commits November 4, 2020 10:29
…<TObject, IEnumerable<TValue>>> selector

Implemented  public static List<TValue> ToList<TValue>(this IQueryableList<TValue> source)
@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #245 (373f90c) into master (744a202) will decrease coverage by 0.29%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
- Coverage   79.12%   78.83%   -0.30%     
==========================================
  Files          73       73              
  Lines        2438     2447       +9     
  Branches      356      357       +1     
==========================================
  Hits         1929     1929              
- Misses        432      441       +9     
  Partials       77       77              
Impacted Files Coverage Δ
....GraphQL.Core/Core/Builders/QueryEntityBuilders.cs 60.00% <0.00%> (-13.18%) ⬇️
Octokit.GraphQL.Core/QueryableListExtensions.cs 73.52% <0.00%> (ø)

@jcansdale
Copy link
Collaborator

Hi @MichaelDenwood 👋

Thanks for working on this!

I was wondering if we could separate the fix from the schema update? Ideally I'd like to create a workflow that automatically updates the schema.

…ion<Func<TObject, IEnumerable<TValue>>> selector"

This reverts commit 3923872.
…ression<Func<TObject, IEnumerable>> selector, means that Implemented ToList Method here : octokit.graphql.net/Octokit.GraphQL.Core/Queryab
Copy link
Contributor Author

@MichaelDenwood MichaelDenwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as requested

Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

Do you think it makes sense to uncomment this line as part of the PR?
https://github.com/MichaelDenwood/octokit.graphql.net/blob/master/Octokit.GraphQL/Model/User.cs#L221

Could we also add an integration test to ViewerTests?

Something along the lines of this might work:

        [IntegrationTest]
        public async Task Viewer_Has_No_OrganizationVerifiedDomainEmails_For_Octokit()
        {
            var query = new GraphQL.Query().Viewer.Select(user => user.OrganizationVerifiedDomainEmails("octokit").ToList());

            var emails = await Connection.Run(query);

            Assert.Equal(0, emails.Count);
        }

I haven't tested it. You probably know more about how this works than I do! 😉

@MichaelDenwood
Copy link
Contributor Author

MichaelDenwood commented Nov 11, 2020

I can do that but it will take me some time. I am on several large projects at the moment. On a side note, I am using this product to connect to both GitHub and GitHub Enterprise. In order to make this work, I had to add a field to the user class, suspendedDate, to support my needs for GitHub Enterprise. I did this in my own fork that I am not merging back. I wanted to bring this to your attention if you didn't already know that there are schema differences between the GitHub cloud offering and the on premises appliance.

@jcansdale
Copy link
Collaborator

I can do that but it will take me some time.

Thanks for letting me know. I'm going to merge this PR and add some tests in a follow-up PR. 😄

Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this merged and add some tests in a follow up PR.

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.

2 participants