Add selectClass()/selectMethod() variants in DiscoverySelectors that take a ClassLoader

[kriegfrj]

• Implements Support explicit ClassLoader in ClassSelector and MethodSelector #1987

[sbrannen]

Hi @kriegfrj,

It looks like you're almost done with this PR (rebased on main and updated). 👍

So, it appears that only an entry in the release notes is missing.

Do you think you'll have time to finish this PR this week?

If not, someone in the team can take merge the PR and add an entry in the release notes.

Please just let us know about your availability for completing this issue.

Thanks!

[sbrannen]

Whenever we introduce an overloaded constructor or method, we do our best to append optional arguments to the formal parameter list instead of prepending them.

This aligns with conventional best practices for API design.

In light of that, please modify all constructors/methods in this PR to accept the ClassLoader last in the formal parameter list (unless there is a compelling reason not to do so).

Thanks

[kriegfrj]

Whenever we introduce an overloaded constructor or method, we do our best to append optional arguments to the formal parameter list instead of prepending them.

This aligns with conventional best practices for API design.

In light of that, please modify all constructors/methods in this PR to accept the ClassLoader last in the formal parameter list (unless there is a compelling reason not to do so).

I think the reason I put them first (it's been a while!) was that, from a domain perspective, all the other argument lists go from least specific selector segment to most specific (eg, (class, method) or (class, method, parameters) ). In keeping with that style seemed to make the most sense to put the class loader first.

I started putting it last, and realised that it causes a source-level compatibility as some of the tests started to fail to compile due to the ambiguous overloads when supplied with a null parameter (this may also be part of the reason I put them first). Having looked at it though, I think in practice that this will be ok because the preconditions all insist that the String arguments are non-null, so there should be no existing code out in the wild that could run afoul of this newly-introduced source ambiguity. So I think in practice only the test cases that are testing the preconditions (attempting to pass in a null) are affected.

I've also updated the release notes.

I also realised that, since I submitted the original PR, we also have the NestedClassSelector and NestedMethodSelector which might need similar treatment... I'm not sure if I'll have time for that too.

[sbrannen]

Thanks for making the requested changes and updating the release notes. 👍

I also realised that, since I submitted the original PR, we also have the NestedClassSelector and NestedMethodSelector which might need similar treatment... I'm not sure if I'll have time for that too.

Good catch.

I opened #3298 to track that.

[sbrannen]

I think this PR looks good now.

I'll make any additional changes after merging.

[sbrannen]

This has been merged into main in c12634c and revised in 142476c.

Thanks