Ddansby/issue 154 add delete branch and tag methods #156
Conversation
|
@ktrysmt done. thank you |
|
hey @ktrysmt FYI, not sure if you saw my last message, but the branch/PR should be good to merge now. let me know if you need anything else. |
|
Hi, @DataDavD . It would be helpful for me if you could rebase and summarize the commits. |
|
my mistake. No problem! |
|
hey @ktrysmt I'm a bit of a rebase noob tbh. I had planned to start rebasing from my initial commit for this branch (i.e. Thanks in advance for your help and patience. Best, |
|
@DataDavD Thanks for your operation. Sorry maybe rebase is not necessary. I think what this probably need squash of commits. |
|
No worries at all. I think you are right. However, going forward, I will start squashing any commits/merge commits I have before pushing to the remote so that the history is a little cleaner for my PRs. Let me know if you need any more help from me on this. thanks again! |
|
Hey @ktrysmt, I was wondering if you started thinking about how we can merge this PR without breaking existing code. Since this combines Tag/Branch options into the single RefOptions this will cause breaking changes. However, I think its beneficial as it simplifies the code and closer aligns with git data model. What is the process in |
|
@DataDavD Certainly this PR contains destructive changes. However, the major version number for this project is v0. Therefore, it is permissible to add destructive changes to the mainline. But it's not so good to release it without checking it in detail. So, I'll try to test it. |
|
By the way, this branch has conflicts, please resolve it and push the clean codes. @DataDavD |
|
Sounds good @ktrysmt , completely agree with you. I can assist with any test additions as well. |
|
@DataDavD Would you please fix the conflict? |
|
Hey @ktrysmt im out of town but will be back Monday and be able to resolve the conflicts then. I believe it's related to someone getting a PR merged for their version of BranchDelete but should be able to easily combine theirs with these ref additions/changes. I'll let y'a know onces their resolved and the PR is good again. Thanks again!!! |
|
I understood. Thank you for your reply! @DataDavD |
|
hey @ktrysmt sorry for the delayed action. I had to travel again and have been very busy with work and life (holidays are happening around here) so I haven't finished resolving the conflicts yet. I was about done but realized I messed up one of my applied changes during my rebase merge so I need to redo it again. I should be done tomorrow. However, I'm curious if @chmouel would be affected if we reverted his DeleteBranch functionality to my original implementation provided in this PR (The main conflict is the fact that the DeleteBranch from @chmouel was approved/merged after this PR was approved (but not merged), so need to decide the original implementation; I'm inclined to opine towards my implementation). It should be fine, but I wanna make sure it won't impact @chmouel in a negative manner (would also appreciate your thoughts here @ktrysmt). |
|
@DataDavD thanks for letting me know! yeah my code will probably be affected but i'll update it! cheers 👍🏻 |
|
It would be nice for me if you could do some test just to be sure after the conflict's resolution. |
|
Sounds good @ktrysmt. Sorry for the delay, been super busy with life and work and haven't been able to do any personal work. I should have this done before next weekend, most likely much earlier. I'll let you and @chmouel know when it's ready so that @chmouel can take a look and have a buffer for him to adjust any code impacted by this change. |
This commit renames the "BranchName" field to "Name" for the RepositoryBranchOptions and adds the "Name" RepositoryTagOptions to allow for deleting branch and tags with the same option struct types.
This commit creates the delete tag and branch methods. It also renames the repo tag options variable name from rbo to rto for code readability and understanding. This repo also renames the BranchName field references to Name.
This commit refactors the existing RepositoryBranchTarget and RepositoryTagTarget types to a single RepositoryRefTarget since tags and branches are both refs and the two Target variables represented the same type anyways. This will also help us move to removing branch and tag specific options types into a single refs options type. See ktrysmt#155 and ktrysmt#153 (comment)
This commit tests the DeleteBranch and DeleteTag repo methods. It also updates the ListRefs part of the tests to ensure that it also properly lists tags as well as branches.
DataDavD
force-pushed
the
ddansby/ISSUE-154-add-delete-branch-and-tag-methods
branch
from
98982fe to
57c51b0
Compare
|
@DataDavD that's correct! my project is go.modded so when i'll start updating with |
|
hey @ktrysmt sorry for spam, but I was wondering if you saw this, wanna try and merge before more conflicts arise. Happy to discuss merges/update options async here too |
This PR closes #154. It specifically adds the DeleteBranch and DeleteTag methods.
Note this PR should not be merged until #155 is merged since it uses the code from that PR for its tests.
It also works towards removing branch and tag specific options and moving to a single refs options (see #153). Furthermore This test also improves the ListRefs tests from #155 by adding a part that tests the creation of a Tag and then also tests the new DeleteBranch and DeleteTag methods as they are used in the tear down of the ListRefs tests (they are used to delete the test tag/branch).
I believe this is good to be approved/merged, but I am interested to hear opinions about how I currently handle errors/responses from DELETE http methods. Currently, the Bitbucket API returns an error json/map[string]interface{} type if there is an error and returns and 204 response code if the DELETE is successful.
Currently, in this PR I just processed and returned interface{} similarly to the Delete method for the repository type itself. Do you think we should leave it to the user to handle error responses? Or should I try to handle it more gracefully? Should I worry about providing more intuitive success responses; or just leave it as a typical delete method in that only return a response if there is an issue? Currently, for testing, I search that the response doesn't include the error type which is indicative that there was an issue deleting a branch/tag for any of the reasons specified in the documentation (for example https://developer.atlassian.com/bitbucket/api/2/reference/resource/repositories/%7Bworkspace%7D/%7Brepo_slug%7D/refs/tags/%7Bname%7D#delete)