Automatic paramter conversion #175
Conversation
|
My mistake, fix it in during today |
There was a problem hiding this comment.
Can you talk a bit about how this might impact existing users of this library?
Perhaps this might make sense implemented as a second provider that targets objects instead of augmenting the existing provider.
I'd like to keep this open for feedback from anyone using this (as I'm not actively doing so right now). I'm very out of the loop when it comes to how to publish Java libs these days.
You're also welcome to fork this and release it under the same name (in your own namespace). That might be best as I can't even recall what's necessary to release this at this point (and I see that the CI automation is failing).
| @@ -34,6 +34,7 @@ dependencies { | |||
| // jakarta.json-api is not exposed on any public members, so does not need | |||
| // to be exposed to use the library | |||
| implementation 'jakarta.json:jakarta.json-api:2.1.3' | |||
| implementation 'com.fasterxml.jackson.core:jackson-databind:2.16.0' | |||
There was a problem hiding this comment.
Is there a jakarta lib that would work for this rather than jackson? (and is that a reasonable idea)
Possibly this: https://projects.eclipse.org/projects/ee4j.jsonb
The general goal of this library was to avoid taking a dependency on any specific JSON api, and I think at the time the jakarta ones were the closest to being a general spec. It's possible that jackson is more widely used, but it's been a long while since I've written any Java or looked at this library. You probably have more up to date context about these things than I do currently (or could perhaps dig into this to understand whether this is the right approach).
| @@ -1,7 +1,7 @@ | |||
| # junit-json-params | |||
|
|
|||
| A [Junit 5](http://junit.org/junit5/) library to provide annotations that load | |||
| data from JSON Strings or files in parameterized tests. | |||
| singleObject from JSON Strings or files in parameterized tests. | |||
There was a problem hiding this comment.
Maybe "data as either values or deserialized into a a single object" would make more sense (If you're not a native English speaker, slap this doc into an LLM and check what makes sense there.)
| } | ||
| } | ||
|
|
||
| private static Stream<JsonValue> values(Reader reader) { | ||
| private static Stream<JsonValue> values(Reader reader, boolean arrayClass) { |
There was a problem hiding this comment.
Use this.isArrayClass directly in this method rather than calling a private method with value which is stored in the class.
| Stream<JsonValue> values = Stream.of(structure); | ||
| if (structure.getValueType() == JsonValue.ValueType.ARRAY) { | ||
| values = arrayClass ? values : structure.asJsonArray().stream(); | ||
| } | ||
| return values; |
There was a problem hiding this comment.
I think this is possibly simplified as:
bool isArray = (structure.getValueType() == JsonValue.ValueType.ARRAY);
return (isArray && !this.isArrayClass)
? structure.asJsonArray().stream
: Stream.of(structure);Does that make sense? That avoids calling Stream.of unless it's needed. It's unlikely this is a perf issue, but I think it's clearer. Thoughts?
|
|
||
| public class JsonConverter implements ArgumentConverter { | ||
| private static final Logger logger = Logger.getLogger(JsonConverter.class.getSimpleName()); |
There was a problem hiding this comment.
What's the impact of adding logging to the tests? Does it get in the way of the output / cause any problems?
I suspect that it might be worth removing this to avoid problems with code that also logs. (But again, it's been a while since I looked at Java and can't recall where J.u.l fits in to things).
There was a problem hiding this comment.
Looking at how the tests change, it seems like this change would break any code that uses this library. Is that right?
Would it be worth making this change as an additive approach instead of one that replaces the implementation? This would make it easier for existing projects to continue to use this library as it is and newer ones to use the object approach.
|
Hi again, I think will good to remove that code and add just second converter, example JsonPojoConverter, to map JsonObject to pojo, what do you think? |
Sounds good, but see my comment about making a fork of this repo to continue work on. I'd recommend taking that approach (and seeing if you can find some other users of this tood). I'm happy to point others towards your fork, but I'd prefer not to hand over this project to an unknown person(due to the stuff that happened with xz a while back) |
|
I agree with all, lets close PR |
Hi, I impl automatic convertion to class in params, it's optional functionality in @JsonSource, but not in @JsonFileSource, tests also was rewrote. (Related to #30 issue). Hope this changes will be useful for you.