Add createAssert(ValueProvider) to AssertFactory
#3377
Conversation
|
I believe the reported binary incompatibility issue is a false positive – raised siom79/japicmp#384. |
Qodana Community for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at qodana-support@jetbrains.com
|
scordio
force-pushed
the
gh-3368
branch
2 times, most recently
from
6351e68
to
684063f
Compare
|
@snicoll aside from enhancing test coverage, I don't expect major changes to the productive code. Could you check if this is what you expect? About:
I have the feeling such a change would be binary incompatible — not the interface per se but applying it consistently in the rest of the library. I'll have a better look in the upcoming days. About:
I haven't introduced it yet but I still have the feeling it would be valuable. I'll probably do it once the rest is covered. |
I want to acknowledge I've seen this but I wasn't able to spend time on this yet. It's high on my list. Thanks a lot for your efforts! |
No problem at all and good luck with your speech tomorrow! I'll be around if you'd like to chat about this topic 😉 |
This reverts commit d6cb373.
|
Looking at the Javadoc of What we need is an Doing this means that The problem currently is that I also wonder if the |
Locally, I introduced an interface that extends I'll polish my changes and raise them as an additional PR on top of this one so that we can look at it. However, reading the rest of your message, I have the impression that you don't want to rely on the existing available methods but only take advantage of the instances in
What do you mean by "if the type is not exposed"? Or, more generally, what kind of implementation do you imagine for In #3368 (comment), you mentioned an approach with default method but that was about having a
Indeed, that's not mandatory and can be kept package-private. |
As expected, the binary compatibility of #3389 is a disaster 🙃 We might introduce the interface but applying it consistently would be postponed until version 4. |
I don't think we need a separate interface. This is what I was suggesting: @FunctionalInterface
public interface AssertFactory<T, ASSERT extends Assert<?, ?>> {
/**
* Creates the custom {@link Assert} instance for the given element value.
* <p>
* Typically, this will just invoke <code>assertThat(actual)</code>
* @param actual the input value for the {@code Assert} instance
* @return returns the custom {@code Assert} instance for the given element value
*/
ASSERT createAssert(T actual);
/**
* Return the target {@link Type} this factory expects, or {@code null} if
* the information is not available.
* @return the element type.
*/
default Type getType() {
return null;
}
}With that, you need to rename As far as I can see this is fully backward compatible. I would not introduce a complete new interface to expose the type as it's required for a very specific case (the one discussed here). If you don't like the getter there in such a high level interface, which I understand, here's an alternative that's also 100% backward compatible: @FunctionalInterface
public interface AssertFactory<T, ASSERT extends Assert<?, ?>> {
/**
* Creates the custom {@link Assert} instance for the given element value.
* <p>
* Typically, this will just invoke <code>assertThat(actual)</code>
* @param actual the input value for the {@code Assert} instance
* @return returns the custom {@code Assert} instance for the given element value
*/
ASSERT createAssert(T actual);
/**
* Creates the custom {@link Assert} instance for the value provided by the
* given {@code valueProvider}.
* @param valueProvider the value provider for the {@code Assert} instance
* @return returns the custom {@code Assert} instance for the value produced by
* the given value provider
*/
default ASSERT createAssert(Function<Type, T> valueProvider) {
throw new UnsupportedOperationException();
}
}Then, any |
|
I'm fine with I must admit I didn't understand how the other alternative would impact One remaining topic because I'm still not sure about it: do you expect your users to rely directly on all the Lastly, would you see an advantage if AssertJ offered native support for parameterized type references, like in the snippet at #3368 (comment)? |
assertj-core/src/main/java/org/assertj/core/api/AssertFactory.java
Outdated
Show resolved
Hide resolved
This reverts commit 6d86a5f.
We discussed that we'll wait for a concrete demand. |
# Conflicts: # assertj-core/src/test/java/org/assertj/core/api/InstanceOfAssertFactoriesTest.java
# Conflicts: # assertj-core/src/main/java/org/assertj/core/api/InstanceOfAssertFactories.java # assertj-core/src/test/java/org/assertj/core/api/InstanceOfAssertFactoriesTest.java
scordio
changed the title
InstanceOfAssertFactory type and raw typecreateAssert(ValueProvider) to AssertFactory
|
@snicoll could you try the latest changes? It should be in line with what we discussed, although I still need to spend some time on it for proper test coverage. Any feedback is welcome! |
|
Looks good. I like the interface that keeps a link between |
|
BTW, we already have |
|
Cool, I hijacked this in private static final DefaultConversionService conversionService = new DefaultConversionService();
@Test
void springConversionTest() {
List<String> values = List.of("123", "456", "789");
// WHEN
ListAssert<Integer> result = list(Integer.class).createAssert(type -> convert(values, type));
// THEN
result.containsExactly(123, 456, 789);
}
@SuppressWarnings("unchecked")
private <T> T convert(Object instance, Type type) {
return (T) conversionService.convert(instance,
new TypeDescriptor(ResolvableType.forType(type), null, null));
}This is one example of many. The json path that was the inspiration for this ask is relying on Jackson to do the conversion based on a |
Qodana Community for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at qodana-support@jetbrains.com
|
scordio
force-pushed
the
gh-3368
branch
4 times, most recently
from
ff2d61d
to
b63aea7
Compare
assertj-core/src/main/java/org/assertj/core/api/AssertFactory.java
Outdated
Show resolved
Hide resolved
|
I'm still unsure how much the Javadoc should elaborate on the concrete usage of the new @snicoll I will merge the changes and release them as part of 3.26.0 in the close future. I would be very interested in seeing how this will be used so that I can derive a meaningful example for our Javadoc or docs 🙂 |
Closes #3368.
Open points:
InstanceOfAssertFactoryconstructorcreateAssert(ValueProvider)ofInstanceOfAssertFactoryInstanceOfAssertFactoriesinstances and relatedcreateAssert(ValueProvider)behavior.DefaultConversionServicefromspring-core