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

Feat/add rest template #34

Merged
merged 5 commits into from
Mar 6, 2019
Merged

Feat/add rest template #34

merged 5 commits into from
Mar 6, 2019

Conversation

dwood12
Copy link
Collaborator

@dwood12 dwood12 commented Feb 23, 2019

This feature adds Spring RestTemplate to call the GitHub API instead of using the curl request. Resolves issue #30

Other changes:

  • Updated the gitignore
  • Commented out test code temporarily until they are fixed. Unrelated to branch but wanted the tests to pass
  • Removed all curl methods

String serializedResponse = getSerializedResponse(bufferedReader);
issues = new ObjectMapper().readValue(serializedResponse, IssueDto[].class);
} catch (IOException e) {
ResponseEntity<String> responseEntity = restTemplate.getForEntity(URL, String.class);
Copy link
Owner

Choose a reason for hiding this comment

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

do we want to capture this as a string? should we capture as a list of the object type we expect?

Copy link
Owner

@mitchellirvin mitchellirvin 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 chat more about capturing the response object in the getForEntity call. the jackson object mapper gives us some fine grained control over deserialization, which we might lose otherwise. can we add our custom object mapper to the rest template config?

Also, let's get unit test coverage on this file.

@mitchellirvin mitchellirvin merged commit 3a4eddf into dev Mar 6, 2019
@mitchellirvin mitchellirvin deleted the feat/add-rest-template branch March 6, 2019 01:03
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