Changing Capability.addResource() to take varargs as last parameter

[Thunderforge]

Before this change, there were three methods:

• addResource(String resource)
  • Example: addResource("foo")
• addResource(String resource, String op)
  • Example: addResource("foo", "bar")"
• addResource(String resource, String[] ops)
  • Example: addResource("foo", new String[] {"bar", "baz"})"

With this change, all three are replaced with:

• addResource(String resource, String... ops)

Which allows the user to do all three of the above without modifications to their code:

• addResource("foo")
• addResource("foo", "bar")"
• addResource("foo", new String[] {"bar", "baz"})"

While also allowing:

• addResource("foo", "bar", "baz")"

This will not break any existing code. It's also makes it consistent with other methods in this project such as EventEmitter.emit(Event, Object...)

[Thunderforge]

The first check failed with the following:

> Task :android:compileReleaseSources
/home/runner/work/ably-java/ably-java/lib/src/main/java/io/ably/lib/transport/Defaults.java:3: error: cannot find symbol

> Task :android:javadoc
import io.ably.lib.BuildConfig;
                  ^
  symbol:   class BuildConfig
  location: package io.ably.lib
1 error

However, I didn't touch either Defaults or BuildConfig. Can someone explain what's going wrong and how to address it?

[QuintinWillison]

We're aware that the assemble workflow is failing and were planning to get it fixed sooner rather than later. You'll observe it's also failing on main branch.

In terms of the change you propose, was there anything in particular that led you to raise this pull request? Were you seeing unexpected behaviour in the library, for example?

[QuintinWillison]

I've removed the assemble workflow to remove that confusion.

Sorry, we shouldn't have had that in place yet.

[Thunderforge]

In terms of the change you propose, was there anything in particular that led you to raise this pull request? Were you seeing unexpected behaviour in the library, for example?

We just started using this library and wanted to add several capabilities. It seemed really un-Java-like to require a String[] to do that (for instance, quite a few methods in java.nio.Files take varargs and not one of them takes a String[]). We also noticed it was also inconsistent with other methods in this library (such as the previously aforementioned EventEmitter.emit(Event, Object...).

So it wasn't a matter of unexpected behavior, just that the method signature isn't consistent with how modern Java typically works and was an added source of confusion.

To reiterate: this change doesn't break any of the previous ways people are using this class, it just allows them to use addResource("foo", "bar", "baz"), meaning they no longer have to create an inline String[] to add multiple ops. It also reduces the maintenance cost by condensing three methods into one.

[QuintinWillison]

Perfect and that makes complete sense the way you explain it.

Thanks for taking the time to raise a PR. We'll take a proper look with a view to getting this merged. 😁

[martin-morek]

LGTM

[QuintinWillison]

Hi @Thunderforge - in case you didn't spot it, this pull request was released in version 1.2.7 yesterday.
Thanks for your contribution. ❤️ Sorry it took so long!

[Thunderforge]

Great, thank you!