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

Refactor client to consistently use GraphQL API. Refactor tests #47

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chrisreddington
Copy link
Collaborator

This pull request includes significant changes to the github/client.go and main_test.go files to transition from using REST API calls to GraphQL queries. Additionally, it refactors the tests to use the new MockGitHubClient from the mocks package, reducing some duplication across both of those files.

Throughout this PR, I'll also aim to do further cleanup in separating out those concerns.

…utilities to consolidate duplication in tests
@Copilot Copilot bot review requested due to automatic review settings December 15, 2024 11:45
@chrisreddington chrisreddington changed the title Refactor client to consitently use GraphQL API. Refactor tests Refactor client to consistently use GraphQL API. Refactor tests Dec 15, 2024
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 8 changed files in this pull request and generated 1 comment.

Files not reviewed (3)
  • github/client.go: Evaluated as low risk
  • github/client_test.go: Evaluated as low risk
  • main.go: Evaluated as low risk
Comments suppressed due to low confidence (2)

types/types.go:42

  • The removal of the 'Data' field from the ContributionsResponse struct changes the expected structure of the API response. Ensure that all parts of the code that parse this response are updated accordingly to avoid runtime errors.
Login                   string `json:"login"`

main_test.go:22

  • The MockBrowser should return m.Err only if m.Err is not nil to avoid returning a nil error when no error is expected.
return m.Err

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

return nil, m.Err
}
// Always return generated mock data with valid contributions
return fixtures.GenerateContributionsResponse(username, year), nil
Copy link
Preview

Copilot AI Dec 15, 2024

Choose a reason for hiding this comment

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

The FetchContributions method should return the MockData field if it is set, rather than always generating new mock data.

Suggested change
return fixtures.GenerateContributionsResponse(username, year), nil
// Return MockData if it is set, otherwise generate new mock data
if m.MockData != nil {
return m.MockData, nil
}
// Always return generated mock data with valid contributions

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

1 participant