docs: Clarify CreateTree semantics and TreeEntry usage
#3877
Conversation
gmlewis
changed the title
CreateTree semantics and TreeEntry usage
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3877 +/- ##
=======================================
Coverage 92.48% 92.48%
=======================================
Files 200 200
Lines 14564 14564
=======================================
Hits 13469 13469
Misses 895 895
Partials 200 200 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @rogpeppe!
I saw the "sha": null first and wanted to make it render distinctively, so then thought we might as well try to be consistent, so if you could go through and back-tick all the code elements, that would be great, thanks! (I stopped flagging them, but you can do the rest.)
The we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.
cc: @stevehipwell - @alexandear - @zyfy29
github/git_trees.go
Outdated
| // When used with [GitService.CreateTree], set Content for small text files, | ||
| // or set SHA to reference an existing blob (use [GitService.CreateBlob] for | ||
| // binary files or large content). To delete an entry, set both Content and SHA | ||
| // to nil; the entry will be serialized with "sha": null which the API interprets |
There was a problem hiding this comment.
| // to nil; the entry will be serialized with "sha": null which the API interprets | |
| // to `nil`; the entry will be serialized with `"sha": null` which the API interprets |
There was a problem hiding this comment.
Done the second of those two.
| // or set SHA to reference an existing blob (use [GitService.CreateBlob] for | ||
| // binary files or large content). To delete an entry, set both Content and SHA |
There was a problem hiding this comment.
| // or set SHA to reference an existing blob (use [GitService.CreateBlob] for | |
| // binary files or large content). To delete an entry, set both Content and SHA | |
| // or set `SHA` to reference an existing blob (use [GitService.CreateBlob] for | |
| // binary files or large content). To delete an entry, set both `Content` and `SHA` |
There was a problem hiding this comment.
I had a look at current precedent and it seems to me that it fits more with Go convention and existing conventions in this package not to quote field names or the Go value "nil". For example all the Get* methods mention the field in question without quoting it, and there are many examples of using "nil" in doc comments in the stdlib without quoting.
I totally agree with backquoting the JSON fragment however.
Happy to make the changes anyway if you decide otherwise.
There was a problem hiding this comment.
I had a look at current precedent and it seems to me that it fits more with Go convention and existing conventions in this package not to quote field names or the Go value "nil". For example all the Get* methods mention the field in question without quoting it, and there are many examples of using "nil" in doc comments in the stdlib without quoting.
I totally agree with backquoting the JSON fragment however.
Happy to make the changes anyway if you decide otherwise.
True, that's a good point. Go ahead and follow the Go convention. Thanks, @rogpeppe!
There was a problem hiding this comment.
Following the Go convention, you would wrap Content and SHA in square brackets, right?
[Content][SHA]
There was a problem hiding this comment.
That wouldn't fit with the doc links syntax. I guess we could use [TreeEntry.Content] and [TreeEntry.SHA] instead, but given that other doc comments in this package already refer to fields without such a qualifier, that seems a bit unnecessary to me. Happy to change to do it anyway if you think it's worthwhile.
There was a problem hiding this comment.
OK, I'm fine with that.
To fix this properly, it would probably mean that we should make the entire repo consistent with the Go doc links syntax, which would be a huge undertaking. If you feel like filing an issue, that would be great, but let's proceed as you currently have it. Thanks, @rogpeppe!
Document the merge behavior when baseTree is provided, how to delete files and directories by setting SHA and Content to nil, and that duplicate paths result in last-entry-wins behavior. Also document on TreeEntry how to use Content vs SHA, referencing CreateBlob for binary files, and that Type/Mode are ignored when deleting. Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
rogpeppe
force-pushed
the
improve-createtree-docs
branch
from
3dc95f0 to
d61fb68
Compare
|
Thank you, @stevehipwell! |
3 participants
Summary
This PR improves the documentation for
CreateTreeandTreeEntryto clarify several behaviors that are not obvious from the existing docs:baseTreeis provided, entries are merged with that tree - paths not mentioned inentriesare preserved from the base treeSHAandContentto nil; this works for both files and entire directoriesTypeandModefields are ignored by the API; onlyPathis requiredContent(small text files) vsSHA(reference to existing blob), with a reference toCreateBlobfor binary filesThese behaviors were verified experimentally against the GitHub API.
Test plan
go test ./github/... -run TestGitService_.*Tree)🤖 Generated with Claude Code