Skip to content
This repository has been archived by the owner on Jan 27, 2025. It is now read-only.

Fix path wildcards in combination with arrays. #142

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

blackwinter
Copy link
Member

Resolves #115 and #119.

NOTE: The updated test MetafixRecordTest.appendWithAsteriksWildcardAtTheEnd() was probably wrong in the first place; the original comment expected the other values as well, but the actual review (which was the basis for the unit test) did not.

Previously, when a wildcard matched multiple fields of which at least one was an array, `Hash.get()` returned a nested array (e.g., `[[Mary, University], Max]`).

Now, matched arrays are flattened (i.e., `[Mary, University, Max]`).
@blackwinter blackwinter requested a review from fsteeg February 10, 2022 13:38
@blackwinter blackwinter linked an issue Feb 10, 2022 that may be closed by this pull request
Copy link
Member

@fsteeg fsteeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these enabled test cases 🙌

@fsteeg fsteeg assigned blackwinter and unassigned fsteeg Feb 11, 2022
@TobiasNx
Copy link
Collaborator

Resolves #115 and #119.

NOTE: The updated test MetafixRecordTest.appendWithAsteriksWildcardAtTheEnd() was probably wrong in the first place; the original comment expected the other values as well, but the actual review (which was the basis for the unit test) did not.

I have checked that test, it is not wrong the new output seems wrong imo.

The comment suggested/asked how to handle asterisk at the end of an wild-card if it does include all variations of the whole path incl. the subfields.

Now the behaviour seems weird.

input is this:

{
	"animals": ["dog", "cat", "zebra"],
	"animal": "bunny",
	"animols": {
		"name": "bird",
		"type": "TEST"
	},
	"ANIMALS": "dragon and unicorn"

}

Fix:


copy_field("ani*", "stringimals[].$append")

set_array("strongimals[]")
copy_field("ani*", "strongimals[].$append")

ani* => as expecded would only match with the field "animal"

my comment asked if the behaviour of fix would be the same as morph. then it would match with "animals", "animal", "animols" and include other values.

Now the result includes "animal" and "animals" and transforms them to simple string values and does some strange behaviour from the other tests since it does not provide index numbers in "stringimals" and adds $append.

If "animals", "animal" and "animols" should be included, so not just simple string-fields but also objects and arrays, the expected would be:

  "stringimals" : {
    "1" : ["dog", "cat", "zebra"],
    "2" :  "bunny",
    "3" : {"name": "bird", "type": "TEST"}
  } ,
  "strongimals" : [ ["dog", "cat", "zebra"], "bunny" ,  {"name": "bird", "type": "TEST"}]

TobiasNx added a commit to TobiasNx/fix-FunctionalReview-Testing that referenced this pull request Feb 14, 2022
@blackwinter
Copy link
Member Author

blackwinter commented Feb 14, 2022

my comment asked if the behaviour of fix would be the same as morph. then it would match with "animals", "animal", "animols" and include other values.

No, those are two different things:

  1. Do Fix path wildcards match the behaviour of Morph wildcards in that they extend to the whole path?
  2. What happens if a Fix path wildcard matches an array or a hash?

As for 1., we (well, I) decided against it (see my answer to your comment); Fix path wildcards only ever match a single path segment.

The behaviour for 2. is what changed here. Previously, the array (animals[]) would simply get stuck in a dead end, that's why IMO the test was wrong.

The pattern ani* matches all field names animals[], animal and animols, so it's basically equivalent to

copy_field("animals[]", "stringimals[].$append")
copy_field("animal", "stringimals[].$append")
copy_field("animols", "stringimals[].$append")

The question then becomes whether each of these cases is implemented correctly (or at all). And copy_field("array", "new_array[].$append") already works, that's not new (see unit test on master: 46ced0b). Only the last case (copy_field("hash", "new_array[].$append)) does not work at the moment.

@TobiasNx
Copy link
Collaborator

TobiasNx commented Feb 14, 2022

my comment asked if the behaviour of fix would be the same as morph. then it would match with "animals", "animal", "animols" and include other values.

No, those are two different things:

1. Do Fix path wildcards match the behaviour of Morph wildcards in that they extend to the _whole_ path?

2. What happens if a Fix path wildcard matches an array or a hash?
  1. Yes that was the reason I modeled the test like this and not include subfields incl. array-fields.
  2. Agreed. Also agree that the object/hash handling is missing.

But also how array and hash are handled is relevant.

But this could be a problem of the fix and not of your changes.
(It is a general problem: https://metafacture.org/playground/?flux=PG_DATA%0A%7Cas-records%0A%7Cdecode-json%0A%7Cfix%0A%7Cencode-json%28prettyPrinting%3D%22true%22%29%0A%7Cprint%0A%3B&fix=copy_field%28%22a%5B%5D%22%2C+%22z%5B%5D.%24append%22%29&data=%7B%0A++++%22a%22%3A+%5B%22b%22%2C+%22c%22%2C+%22d%22%5D%2C%0A++++%22z%22%3A+%5B%5D%0A%7D&active-editor=fix)

copy_field("animals[]", "stringimals[].$append")

operates like I would expect copy_field("animals[].*", "stringimals[].$append")

@blackwinter
Copy link
Member Author

But this could be a problem of the fix and not of your changes.

Exactly, this is independent of the changes discussed here as it's already working this way.

@TobiasNx
Copy link
Collaborator

TobiasNx commented Feb 14, 2022

But this could be a problem of the fix and not of your changes.

Exactly, this is independent of the changes discussed here as it's already working this way.

my questions are solved. +1
I opened another ticket for the array handling metafacture/metafacture-core#604

@blackwinter blackwinter merged commit f53234c into master Feb 14, 2022
@blackwinter blackwinter deleted the 115-119-fixPathWildcardsWithArrays branch February 14, 2022 12:29
blackwinter added a commit that referenced this pull request Feb 16, 2022
Previously, when a wildcard matched multiple fields of which at least one was an array of hashes, `FixPath.findIn()` returned a nested array (e.g., `[[{value=>Mary}, {value=>University}], {value=>Max}]`).

Now, matched arrays are flattened (i.e., `[{value=>Mary}, {value=>University}, {value=>Max}]`).

See also #142.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants