@bitwiseman I was finally able to do some testing. From my perspective, it's all good: I ported Quarkus GitHub API and Quarkus GitHub App to it with only very minor changes and tested our GitHub bot both in JVM mode and native.

Only one thing comes to mind: it seems you dropped the unbridged artifact? I think you should reinstate it even if for now you don't have bridges as you will add some at some point and they are going to be problematic again for Mockito users.

Oh and also, it seems you didn't drop all the IOException that are extremely annoying when coding with the API. It has been one of my major complaint over time, as it's a bit painful to have to throw IOException everywhere.

In some cases, we even have IOException for methods that are definitely not throwing anything such as:

I think we have a unique opportunity here to clean this up.

BTW, I'm aware it's going to require app changes but really it's for the better and given we are doing a major, I think it's probably now or never. People who don't want to handle this in their apps can keep using the old version if they prefer.

Only one thing comes to mind: it seems you dropped the unbridged artifact?

There was a glitch that I noticed a few weeks after standard release of 1.325. I believe I fixed it with bfa107f and 19ba058 .
Maven Central has 1.325 and 1.326 under https://mvnrepository.com/artifact/org.kohsuke/github-api-unbridged .

Oh and also, it seems you didn't drop all the IOException that are extremely annoying when coding with the API.

This was intentional. As note it is disappointing, but I'm trying to walk the line for minimal number of binary breaking changes for the sake of Jenkins usage. Jenkins' plugin behavior means that compile-time breaking changes are less problematic than binary breaking changes - thus usage of bridge methods. Looking at japicmp - METHOD_NO_LONGER_THROWS_CHECKED_EXCEPTION it appears that removing checked exceptions is a compile-time breaking change but not a binary breaking change. If verified, then I'd be very comfortable removing most checked exceptions as part 2.0.

If verified, then I'd be very comfortable removing most checked exceptions as part 2.0.

I think it would be worth checking. We had quite a lot of complaints about that over time and every time I demo the API at a conference, it does look unnecessarily verbose (which might seem like a small price to pay but we are all struggling to make our API more user friendly and it's definitely worth it).
It's also sometimes quite annoying when dealing with the Stream API and having to catch the exceptions in the lambda. I got hit by that several times and see #1658 for other complaints related to it.

From @MarkEWaite :

I think it is a mistake to sacrifice or significantly disrupt the existing users of the library in order to make the API better for users that are not yet consuming the API. I guess that if the disruption happens and it is too great, then the downstream users (like the Jenkins plugin) can fork the repository and maintain their own fork, but that will reduce the consumers of this API.

I'm not sure I understand what you mean. This change will improve the life of the existing users

I'd extend your phrase "This change will improve the life of the existing users" to say "This change will improve the life of the existing users if and only if they rework the logic in their code to adapt to the changes". In other words, existing users only benefit if they rework the logic in their code. That's what I meant when I said "sacrifice or significantly disrupt the existing users of the library".

then the downstream users (like the Jenkins plugin) can fork the repository and maintain their own fork, but that will reduce the consumers of this API.

They can continue to use the version they are using if they are happy with it. Or they can move to the new version and benefit from the better DevEx.

Being locked on a specific outdated version has been a bad experience in the Jenkins project. It creates a situation that looks like a fork of the upstream repository without the clarity of actually forking the upstream repository and releasing it as a new artifact.

When a security issue is detected in the upstream library, the consumer of the specific outdated version must either perform all the work to update to the fixed version (adapting to the behavior change while also performing a security update) or they must fork the repository and backport the security fix to a new release that is based on the specific outdated version.

I understand your point but what you're saying is basically "never do a new major version because changes to the API will be too disruptive for the existing ecosystem".

I'm not saying the situation is ideal, it's definitely not. This change is quite nasty. But it's a bit of a shame that even for a major version, we can't do breaking changes when something is actually bad in the API.
Note that I have quite a lot of code using the GitHub API so I know it's going to have an impact on me too. I'm not exactly excited about it but I think this API would be a lot easier to use in the future if we did this change.

I agree it's going to be painful for a while but it might be worth it if it makes the life of our future selves and all the new users better.

When a security issue is detected in the upstream library, the consumer of the specific outdated version must either perform all the work to update to the fixed version (adapting to the behavior change while also performing a security update) or they must fork the repository and backport the security fix to a new release that is based on the specific outdated version.

That's why you usually push security fixes to multiple release streams - at least when they are not too disruptive.

Again, I won't be the one making the decision and I understand your point, I'm just trying to challenge things a bit because well a major version doesn't happen very often and they are usually quite unique occasions to improve things significantly.

I agree with Guillaume, if we can't afford to introduce breaking changes even in semver release versions, then, when can we?

BTW, if we decide to go with it, I think we probably need to review very carefully the typology of the changes, categorize them and make sure the semantic is good and consistent for all of them.

I'm preparing Devoxx and will be off all next week but happy to discuss it further after Devoxx.

Major version != free pass for all breaking changes

I understand your point but what you're saying is basically "never do a new major version because changes to the API will be too disruptive for the existing ecosystem".

No, what's being said is that just because a particular breaking API change has advantages doesn't mean it must be included in a major version bump.

I agree with Guillaume, if we can't afford to introduce breaking changes even in semver release versions, then, when can we?

Not all major versions are huge changes. The main reason I'm pulling the trigger on 2.x is to reduce ongoing maintenance headaches: Java 8, URLConnection, and ancient bridge methods. A major version bump is is not justification in itself for any particular change, it only means more changes can be considered.

Streaming-friendly API is an important feature, now how to implement it

I have not heard anyone speak against having a Streaming-friendly API. The only thing I've said is that it can be implemented without breaking existing contracts. Yes, that will add code to the project, but from my perspective that code has low complexity. I'm even comfortable saying we should move to have streaming-friendly code being the default for new features. We progressively deprecate the old code and move the project in that direction so that in a year or so we can cut a 3.0 that drops the old style.

Not all API problems related to exceptions are the exceptions fault

This method is a good example of having to handle simple logic with exception control flow: https://github.com/quarkusio/conversational-release-action/blob/main/src/main/java/io/quarkus/bot/release/step/CreateBranch.java#L111 . I'm not sure it actually helps making things readable and knowing what is a normal error case (this thing doesn't exist) and what is not (I was unable to contact GitHub). It's definitely not fluid when writing code (and having to refer to the Javadoc for every call you write is definitely not ideal).

Especially since I would expect an exception only if for instance I was unable to contact GitHub, not if the thing doesn't exist. The fact that a label doesn't exist is a perfectly valid case from a Java API POV. It shouldn't be an error.

Ah! This is particularly interesting case. Yes, this should be an exception - you asked for an instance of a specific branch (or label, or whatever) and none was returned. The API doesn't know that you want to say is, "If branch A doesn't exist create it" or "If label A exists, rename it. What we should be talking about is not removing exceptions, but making a more expressive API overall. For instance, anything that has a get* or a list* should automatically have an exists* method. But the way the API is written currently makes that a pain to implement. What if your code could look like this:

if (!repository.branch().name(releaseInformation.getBranch()).exists()) {
 try {
 // If get() fails, we want an exception, not a null.
 String sha = repository.branch().name(releaseInformation.getOriginBranch()).get().getSHA1();
 repository.ref().create()
 .name("refs/heads/" + releaseInformation.getBranch())
 .sha(sha)
 .done();
 } catch (Exception e2) {
 throw new IllegalStateException(
 "Unable to create branch " + releaseInformation.getBranch() + " from "
 + releaseInformation.getOriginBranch() + ": " + e2.getMessage(),
 e2);
 }
}

Rather than getting stuck on modifying the existing methods, let's talk about creating an API that is better.

Hey Liam,

Sorry, things have been a bit crazy on my side.

I completely agree with you, I think most of the misunderstanding was about the goals of this version 2.

I'll work on a proposal as time permits, discuss it with my colleagues and try to draw what could be done for the future.

Cheers!

I have to admit that I'm hitting a problem with the PageIterable.
I would like to be able to say that for any GHRepository#queryPullRequests()#list(), I return some content. I could precise the toList() or toArray() method, but for some implementation I could prefer to use the splitIterator() for a Stream implementation.

The problem is that the PageIterable implementations are usable, and implementing one is problematic as the PageIterator constructor is package visible only, so I cannot use it in a PageIterable#_iterator() implementation.

I don't have a solution right now or an idea of a solution but I'll try to think of one.

@alecharp
I've thought about exposing page iterator as public. But I've been looking to coordinate it with other improvements to page iterable. See #1730 and related issues and discussion.

Can you give an example of what you'd like the API to look like?

I tent to like the Stream API.
If we could have a .stream(), so that we can easily manipulate the feedback of the API. It could provide an equivalent of the .toList() or toArray() of the PageIterator.
It could simplify the mocking of such request, no?