ascopes / java-compiler-testing

Write sandboxed integration tests for Java annotation processors and plugins.
https://ascopes.github.io/java-compiler-testing/
Apache License 2.0
12 stars 10 forks source link

Add overloads to `T first, T... rest` methods #721

Closed Marcono1234 closed 1 week ago

Marcono1234 commented 2 weeks ago

First of all, thanks a lot for this library! It is really well documented.

Is your feature request related to a problem? Please describe. At multiple places the API offers methods like method(T first, T... rest). While this is useful to enforce that the user provides at least one element, using such a method when you have a dynamic list of elements is rather cumbersome.

For example, I want to use ManagedDirectory#createFile. I do already have the fragments as List<String>, but to call the method I have to perform rather cumbersome transformations to split off the first element, and convert the remainder to an array:

var fragments = new ArrayList<>(Arrays.asList(javaFile.packageName().split("\\.")));
fragments.add(javaFile.typeSpec().name() + ".java");

// Transform to match `createFile` signature
sourcePath.createFile(fragments.getFirst(), fragments.subList(1, fragments.size()).toArray(String[]::new));

Describe the solution you'd like Ideally these methods would offer a List<T> or Iterable<T> overload as well. This could have a default implementation which performs this cumbersome transformation shown above and then delegate to the existing methods.

Describe alternatives you've considered none

Additional context I assume this is a general API problem not specific to this project, so maybe someone else came up with a better solution for this already than what I proposed above.

ascopes commented 2 weeks ago

This sounds reasonable to me.

Originally my intent was to prevent users passing single elements in, however I can see why this is going to be really annoying in hindsight.

I think changing the API to pass in a single varargs array and having overloads for collection types would be totally reasonable (and able to go under a new major version if the first bit is included).

So the plan would be:

ascopes commented 2 weeks ago

I've merged the changes. Please can you verify this covers what you need by building the main branch locally and trying it out? You may find some breaking changes with how these signatures work now as I've removed the T, T... pattern entirely everywhere and replaced with just T... and appropriate runtime checks as needed.

The new methods use List<String> as the parameter type rather than Collection<String> or Iterable<String> as internally we need to know their size (so they must be at least a Collection<String>), but their order is important and defines their meaning, so passing in Sets as an example should not be allowed.

The example in your description should be covered by these docs: https://ascopes.github.io/java-compiler-testing/io.github.ascopes.jct/io/github/ascopes/jct/workspaces/ManagedDirectory.html#createFile(java.util.List)

List<String> fragments = ...
sourcePath.createFile(fragments);

Hope this helps.

Marcono1234 commented 2 weeks ago

I've merged the changes. Please can you verify this covers what you need by building the main branch locally and trying it out?

Thanks a lot! So far I was not using any advanced JCT features yet, so ManagedDirectory#createFile was the only case where I noticed it, and with your changes that should be solved.

Is it ok for you if I just wait for the next release? No need to rush though; my workaround for the current API works fine, it is just a bit verbose. Or do you want me to test it nonetheless?

ascopes commented 2 weeks ago

Whatever works best for you really, I won't have time for a few weeks to work on the other issue you raised so I likely won't release this properly until I've got it fully covered if that is okay?

ascopes commented 1 week ago

Going to put an RC release out for now as there are some other changes I also want to push. There won't be a stable release until I have time to work on your other issue though. Hope that's ok.

Marcono1234 commented 3 days ago

There won't be a stable release until I have time to work on your other issue though. Hope that's ok.

Of course! Take your time, and thanks a lot for doing this in the first place!