WIP: bitbucket issues support

[snogge]

This is the current status of my work to add bitbucket support to
forge.

I think all issue operations are supported, but it does rely on #196,
which is also included in this PR.

It relies on magit/ghub#97 for mutating
operations.

I would appreciate all feedback on how to improve this code, and tips
on things in the forge architecture that I have missed.

[snogge]

Rebased to not use #196 but rather the proper fix for #195.

[tarsius]

There's a strange typo in the commit message.

The string "bitbake" also appears in code added by later commits.

[tarsius]

Chop of unreadble second decimals for bitbucket

So that means that earlier commits are broken right? I would prefer not to merge any commits that are known to be broken. Instead of this you can add forge--bitbucket-hack-iso8601 in a separate commit, without actually using it right away. That way you can explain why this is necessary in a commit message, without having a commit message that talks about this and other unrelated stuff.

In a similar vain I would add all the ghub--buck-METHOD functions in a single commit, which adds nothing else and doesn't actually start using any of these functins yet. These functions are basically boilerplate and adding one of them in the same commit that starts using it makes that commit look more involved than it actually is. Also if all this boilerplate is added in a single commit, then it is easier to verify that no copy-paste error has occurred when creating one of these functions.

[snogge]

I've moved the addition of forge--buck-* functions to a separate commit, ditto for forge--bitbucket-hack-iso8601.
I've gone over the commit messages but I'm not sure I understand what you mean in your comment about that.

[tarsius]

I've gone over the commit messages but I'm not sure I understand what you mean in your comment about that.

I'm pretty sure you mean bitbucket.org when you write bitbake.org.

[snogge]

Right. bitbake is a tool I use at work, and obviously I use it so much that I type that over bitbucket, and I cannot even see when I'm doing it. I think I've fixed all bitbake mentions now.

[tarsius]

What is the purpose of the ;; checkdoc-params: (forge-bitbucket-repository) comments?

You also added doc-strings for some methods that apply just as much to the other (gitlab) methods of the respective generic functions. I think you should remove those and instead maintain a list of "generic functions that now have at least two methods and which should therefore be explicitly defined eventually, at which point we can also add the doc-string". 😉

[tarsius]

tarsius approved these changes 8 minutes ago

I was merely trying to confirm that you had taken care of changes that I had already requested. I haven't actually reviewed the rest yet. I'll do that when you tell me it is "done" (the same features as for gitlab are supported) or you ask me specific questions.

[tarsius]

Since the other libraries do not have such a header, don't add one here either.

[snogge]

Fixed

[tarsius]

You did not require seq.

[snogge]

Fixed

[tarsius]

Now that I have looked at the code I can say that this generally looks reasonable. Since this mostly based on the Gitlab code that I have written, it's not surprising that I would come to that conclusion. ;-)

You are generally on the right track. If you were waiting for that statement before tackling the remaining big gaps, then you can continue working on this now. If you were hoping I would merge this despite it being rather incomplete, then I have to disappoint you. This should be about on par with Gitlab. So I suggest you use this yourself and fix bugs and implement missing features as you go. And then ping me when it is complete or if you are running into issues.

[snogge]

What is the purpose of the ;; checkdoc-params: (forge-bitbucket-repository) comments?

checkdoc will complain that forge-bitbucket-repository should be mentioned as a parameter - which is clearly wrong. The checkdoc-param comment suppresses that warning. The correct thing would be to make checkdoc handle generic functions, but I didn't want to be distracted.
I'm fine with removing these comments before merging.

You also added doc-strings for some methods that apply just as much to the other
(gitlab) methods of the respective generic functions. I think you should remove those
and instead maintain a list of "generic functions that now have at least two methods and
which should therefore be explicitly defined eventually, at which point we can also
add the doc-string". wink

Lets figure that out when we are closer to merging this.

[snogge]

Now that I have looked at the code I can say that this generally looks reasonable. Since this mostly based on the Gitlab code that I have written, it's not surprising that I would come to that conclusion. ;-)

You are generally on the right track. If you were waiting for that statement before tackling the remaining big gaps, then you can continue working on this now. If you were hoping I would merge this despite it being rather incomplete, then I have to disappoint you. This should be about on par with Gitlab. So I suggest you use this yourself and fix bugs and implement missing features as you go. And then ping me when it is complete or if you are running into issues.

I do have one question that popped up when I began looking at adding pullrequest support.
Bitbucket numbers pullrequests and issues independently, so there will be a pullrequest #N and an issue #N. I noticed that gitlab uses ! to indicate pullrequests, is that for the same reason? Does gitlab do that in their own UI as well? Bitbucket's markdown uses #N for issues and Pull request N
for pullrequests. Is there somewhere to customize that for automatic links in forge?

[tarsius]

noticed that gitlab uses ! to indicate pullrequests, is that for the same reason? Does gitlab do that in their own UI as well?

Yes and yes.

Is there somewhere to customize that for automatic links in forge?

What that? Grepping for ! should reveal the places that have to be adjusted for bitbucket.

[tarsius]

So, how did it go?

[snogge]

I started working on the pull request, but I haven't had that much time to devote to this project recently.

[tarsius]

Take your time. I am not in a hurry and wouldn't be able to review before next year anyway. Just wondering.

[tarsius]

@snogge I would like to either merge or else close this soon. Please let me know what your plans are.

I just rebased and there were no conflicts. There's one compile warning:

forge-bitbucket.el:334:1: Warning: Unused lexical variable ‘user’

[tarsius]

We can of course reopen when/if you start working on this again.

[snogge]

Yeah, I don't see this happening soon. I've spent my emacs dev time on buttercup instead.
One option would be to get this to a state where you would be comfortable merging it without the pull request support, just so someone else could potentially keep going.
Maybe merge the buck changes so that the next guy wont have to go through that,

[tarsius]

If someone else asks about bitbucket support I will point them to your work.

I don't want to merge this without pull-request support because I would then end up having to implement that myself because it would be such a reasonable thing to request once issues and other stuff are supported. I also want to work on other things.

[tarsius]

I have added a faq entry about the forges I plan or don't plan to support. Because I do not plan to implement support for this particular forge I am closing this issue. That doesn't mean that you cannot comment here or shouldn't discourage you from contributing support for favorite forge yourself.