Handle and access repeated fields as arrays #78
Conversation
There was a problem hiding this comment.
I agree that there's a lot left to be discussed. And I'm afraid the (lack of) test coverage isn't helping. For the time being, though, I guess I have to disagree that this "will resolve #65".
I would contend that it might be beneficial (crucial even) to collect (and discuss) an extensive number of tests illustrating the required use cases (a specification of sorts) - and then implement those, and only those.
| case String: | ||
| append(Arrays.asList(newName).stream().collect(Collectors.joining(".")), v.asString()); | ||
| break; | ||
| case Array: |
| } | ||
| } | ||
| else if (fields.length >= 1 && isNumber(fields[0])) { | ||
| final int index = Integer.parseInt(fields[0]) - 1; |
There was a problem hiding this comment.
| result = find(tail(fields)); | ||
| } | ||
| else if (isNumber(fields[0])) { | ||
| final int index = Integer.parseInt(fields[0]) - 1; |
There was a problem hiding this comment.
I agree with that goal, and I was basically trying to go for that with the unit tests here. How can we better collect and discuss these use cases? Maybe something like really short fix snippets provided as use cases, also by @TobiasNx? Which we can run with Catmandu and Metafacture, and which we integrate as unit tests? And thanks for the specific pointers here in the review, I'll work through them to clean this up some more. My main point for eventually merging this is that it fixes the current behavior of merging subsequent entities that have the same name. |
I'm sure you were. It's just that there seems to be quite a lot of implemented behaviour that's not actually covered by any tests. Some might be edge cases (nested find in array with wildcard: As long as we're unsure about the intended behaviour, we shouldn't implement those cases. Conversely, any implemented behaviour should manifest itself somehow in the tests; so we know for sure when and how it might get changed in the future. |
Would it be possible for you to reduce the pull request to the minimal requirements for your OERSI use case? Otherwise, I'll switch my vote to -0 as long as we're clear that any untested behaviour is considered experimental and subject to change (without notice). |
Certainly, that would be great. Ideally even accompanied by equivalent Catmandu tests (cf. hbz/Catmandu@22d7359). The need to discuss referred to behaviour that's not as clear-cut in (or not applicable to) Catmandu. As for the process, I don't have any ideas right now other than pull requests (here and in hbz/Catmandu). |
|
It might even make sense to port the Catmandu test suite in order to ensure compatibility. The next step would be a shared collection of (generic/implementation-independent) test cases which would be executed by both (all) implementations (maybe akin to Parsing JSON is a Minefield: Testing Architecture). |
Co-authored-by: Jens Wille <[email protected]>
|
Thanks again for your feedback, it helped me a lot, and I think this is starting to make sense. I've added TODOs / WDCDs (What Does Catmandu Do) for open questions.
+1 To approach the WDCDs, maybe we should wrap up this PR and focus on starting a setup for testing as you outlined here:
I've always seen actual compatibility as a later step, but maybe it does make sense to start setting up something like that now. To get started, we could work on a modified version (with stuff like quoted variables as currently often required in Metafix etc.). It would still allow us to be guided by the Catmandu concepts, and provide a better way to work with examples (also for @TobiasNx, instead of working on full real-world examples as we currently do in OERSI). |
There was a problem hiding this comment.
OK, so here's my -0 for the sake of progress.
I'll intentionally leave any unresolved conversations (TODOs) open.
I still believe that #65 should not be closed as long as we're unclear about the big picture.
|
I do not know if the issue i found concerning creating arrays (with and without |
Will resolveSee #65.This is far from consistent and a bit messy, but hopefully some steps in the right direction. I noted some TODOs which would result in some refactoring work (mainly around avoiding switch statements by moving functionality into enum elements; as well as consistent handling of special path segments like wildcards and indexes), but to make sure we're moving into the right direction, I'd like to focus on use cases right now.
For our OERSI use case (updated for this state in 8b27e98c), this provides some progress, but I don't think a functional review makes sense at this point. For that, I'd like to continue with the bind scope issues, which are related to #66. To avoid a larger and longer-running branch with these additional changes, I think it makes sense to review and merge the current work in progress at this point.