Add support for deleting files from tree #1678
Conversation
|
@bitwiseman could you please take a look? |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1678 +/- ##
============================================
- Coverage 80.00% 80.00% -0.01%
- Complexity 2223 2224 +1
============================================
Files 213 213
Lines 6727 6732 +5
Branches 365 365
============================================
+ Hits 5382 5386 +4
Misses 1131 1131
- Partials 214 215 +1
☔ View full report in Codecov by Sentry. |
| public GHTreeBuilder delete(String path, boolean executable) { | ||
| TreeEntry entry = new DeleteTreeEntry(path, executable ? "100755" : "100644", "blob"); |
There was a problem hiding this comment.
Is executable required for deletion? Does it need to match the tree object that is being deleted?
If not (if delete works regardless of this value), we should remove the executable parameter here.
There was a problem hiding this comment.
I also wonder what happens if the type doesn't match the item at that path.
https://docs.github.com/en/rest/git/trees?apiVersion=2022-11-28#create-a-tree under properties of tree, shows type can be blob, tree, or commit.
This isn't necessarily a blocker, but it would be good to know if there are scenarios that the current delete() method doesn't handle.
There was a problem hiding this comment.
Is
executablerequired for deletion? Does it need to match the tree object that is being deleted? If not (if delete works regardless of this value), we should remove theexecutableparameter here.
Yes, it's required for deletion as well and it should be not null.
Reference doc says nothing about what value for mode should be used for deletion but my test shows it works with any valid non-null value. So we can use some hadcoded value. I will change this
There was a problem hiding this comment.
I also wonder what happens if the
typedoesn't match the item at thatpath.
I tested type=commit while deleting a blob and it works fine.
|
|
||
| private static class DeleteTreeEntry extends TreeEntry { | ||
| @JsonInclude | ||
| private String sha; |
There was a problem hiding this comment.
This is always going to be null right? I'm surprised there wasn't a warning about sha never being assigned/used...
At minimum add a comment explaining what is going on here.
bitwiseman
left a comment
bitwiseman
left a comment
There was a problem hiding this comment.
A few questions, but otherwise looks great.
|
I've added comments and hardcoded |
2 participants
Fixes #1484
Github API allows deleting files from tree by passing
nullvalue inshafield. Existing implementation skips all null fields during serialization due to@JsonInclude(Include.NON_NULL)annotation. This PR adds separate object for delete operation withshafield which is always serialized.