Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow modifying arrays more specifically #129

Closed
joachimvh opened this issue Nov 9, 2023 · 5 comments · Fixed by #134
Closed

Allow modifying arrays more specifically #129

joachimvh opened this issue Nov 9, 2023 · 5 comments · Fixed by #134

Comments

@joachimvh
Copy link
Member

Issue type:

  • ➕ Feature request

Description:

The idea is to extend the override functionality to somehow allow more flexibility when extending arrays from different configurations. I was thinking of adding another field to an Override object that indicates how array elements should be added to an existing array. Features I was thinking of:

  • Adding a new element before/after another element in the array
  • Adding a new element at a specific index in the array
  • Remove an element with a specific ID (or index) from the array

Not sure yet how feasible it would be to add these as I haven't looked at the array code yet. I will probably look soon at a potential implementation (or decide to give up if the complexity would be too high).

@rubensworks
Copy link
Member

Makes sense indeed.

It may not be super easy to implement though, as we have different types of arrays (explicit RDF lists, multiple predicate values).

@joachimvh
Copy link
Member Author

Having had a look, arrays definitely seem quite feasible. The elements get concatenated immediately through the properties field of the RDF object. And when you overwrite that field you get the correct result. Similarly for key/value lists, although these seem to potentially have multiple entries in the array where the actual elements can be found deeper using the list field. But similarly, everything seem to work when replacing values in there.

One issue will potentially be the verbosity when there are several steps that you want to take. E.g., remove element A, insert element B before X, and replace another field of the same object with something else. Preferably 1 Override could do all this at once, so users don't have to create 2-3 Overrides to change 1 object, with these Overrides then also targeting each other as only 1 can target the main resource. Will look into that.

@joachimvh
Copy link
Member Author

I've had a look at how this could look in practice, taking the above considerations into account. Below is what I'm currently thinking of.

As this would introduce several variations of what you can do with an Override, I'm thinking of deprecating the overrideParameters field and instead introducing overrideSteps, which would take an array of steps and execute those in order on the target object. The @type of that object would then define which action would be taken. Types would be OverrideMapEntry/OverrideListInsertBefore/OverrideListInsertAt/OverrideListRemove, and OverrideParameters for the original Override behaviour.

These objects would make use of the parameters overrideParameter/overrideTarget/overrideValue.

  • overrideParameter points to the specific parameter that contains the list/map in the object.
  • overrideTarget determines where the change occurs.
    • OverrideMapEntry: the key in the map to update.
    • OverrideListInsertBefore: the identifier of the resource before which the new value should be inserted.
    • OverrideListInsertAt: an integer identifying the position where the value should be inserted.
    • OverrideListRemove: the identifier of the resource which should be removed from the list.
  • overrideValue would be the new value to insert at that location. For an OverrideMapEntry this can be omitted to remove the value for that key. This is the only parameter that is necessary for the old OverrideParameters.

This could result in, for example, the following Override:

{
  "@id": "ex:myObjectOverride",
  "@type": "Override",
  "overrideInstance": { "@id": "ex:myClassInstance" },
  "overrideSteps": [
    {
      "@type": "OverrideMapEntry"
      "overrideParameter": { "@id": "MyClass:_mapParameter" },
      "overrideTarget": "mapKey"
      "overrideValue": "newMapValue"
    },
    {
      "@type": "OverrideListInsertBefore",
      "overrideParameter": { "@id": "MyClass:_listParameter" },
      "overrideTarget": { "@id": "idOfElementAlreadyInList" },
      "overrideValue": { "@id": "idOfNewElementToPutInListBeforeTheOtherOne" }
    }
  ]
}

Object ex:myClassInstance, which is an instance of MyClass, and at the very least has parameters mapParameter and listParameter, which take a key/value object and a list respectively. This Override would change the value for key "mapKey" in the map with "newMapValue", and would insert a new element in its list before the one specified.

This seems like a good way to extend Overrides with these new features while minimizing verbosity and also not introducing too many new keywords. It also allows for easily adding new Override types in the future. One big disadvantage is that you can't use type scoping for the parameters in overrideParameter, so the uglier version has to be used, but I don't really see a way around this. We could just take a string as input there which is the name how it appears in the code and then determine the rest based on the fact that we know which class the object is, but that seems more hacky.

As mentioned, I would deprecate the overrideParameters field so there is only 1 way to override, but not remove to prevent breaking existing solutions. So the following example:

{
  "@id": "ex:myObjectOverride",
  "@type": "Override",
  "overrideInstance": { "@id": "ex:myHelloWorldWithOverride" },
  "overrideParameters": {
    "@type": "ex:GreetingsWorld",
    "greetings:say": "GREETINGS WORLD!"
  }
}

would become

{
  "@id": "ex:myObjectOverride",
  "@type": "Override",
  "overrideInstance": { "@id": "ex:myHelloWorldWithOverride" },
  "overrideSteps": [
    {
      "@type": "OverrideParameters",
      "overrideValue": {
        "@type": "ex:GreetingsWorld",
        "greetings:say": "GREETINGS WORLD!"
      }
    }
  ]
}

All of this assuming that it is feasible to support this, but my experience with the initial Override implementation and the feasibility check I did above makes me think this would work.

I'm also not sure if OverrideListInsertAt would actually be that useful, I'm mostly adding it because it is possible and perhaps there are some use cases for it.

@rubensworks
Copy link
Member

Your approach sounds very reasonable to me!

Some minor thoughts:

  • Instead of deprecating overrideParameters, I would just keep it around as a shorter form of overriding.
  • Similar to OverrideListInsertBefore, OverrideListInsertAfter could be useful.
  • OverrideListRemoveAt may also be useful, perhaps with an optional range parameter? (but then something like OverrideListRemoveFrom may be a better name...)

But I certainly like the fact that we can define different types of overriding with this approach.

@joachimvh
Copy link
Member Author

  • Instead of deprecating overrideParameters, I would just keep it around as a shorter form of overriding.

I was planning to just note in the documentation that this still works, but is not really recommended, just because it's a bit inconsistent to have 1 separate solution for this specific type of override. And perhaps even log a warning.

Could also not do that, but I thought it might be confusing if there are 2 ways to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants