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

Adds truncated attribute to TreeResponse. #674

Merged
merged 1 commit into from
Jan 11, 2015
Merged

Adds truncated attribute to TreeResponse. #674

merged 1 commit into from
Jan 11, 2015

Conversation

MicahZoltu
Copy link
Contributor

See https://developer.github.com/v3/git/trees/#get-a-tree for details on the truncated attribute.

I am operating under the assumption that if a property is missing from the json object then the property will be set to its default value (false in this case).

I'm not sure how to test this. If someone can point me at some existing tests that validate the json deserializer behaves correctly in certain scenarios (like when the property is missing) I can add some test coverage. Though, if using an off-the-shelf deserializer then perhaps that wouldn't be testing anything meaningful in the library.

@shiftkey
Copy link
Member

If someone can point me at some existing tests that validate the json deserializer behaves correctly in certain scenarios (like when the property is missing) I can add some test coverage.

We're using SimpleJSON for the serializer - we have some tests here for the related features that we need.

I believe the truncated property should always be present when deserializing the JSON, so unless you can think of some integration tests to add I wouldn't stress about it too much.

@shiftkey shiftkey self-assigned this Jan 11, 2015
@MicahZoltu
Copy link
Contributor Author

I added a json parser test just to prove what happens if an attribute is missing.

I don't know of a public repo returns a truncated tree so that is difficult to integration test. Even with the recursive flag set to 1, getting the full tree for octokit still doesn't truncate. Also, such an integration test would be brittle if GitHub changes the truncation level. Given all of that, I'm going to forgo an integration test unless someone really wants it and can recommend a repo to test against or a way to force truncation.

@shiftkey
Copy link
Member

I don't know of a public repo returns a truncated tree so that is difficult to integration test.

Don't worry about it. Happy to leave this as-is.

shiftkey added a commit that referenced this pull request Jan 11, 2015
Adds truncated attribute to TreeResponse.
@shiftkey shiftkey merged commit e15ad91 into octokit:master Jan 11, 2015
@shiftkey
Copy link
Member

Thanks!

@MicahZoltu MicahZoltu deleted the treeresponse-truncated branch January 11, 2015 22:32
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