Entry Removals in json tag files

[cputnam-a11y]

adds a c:remove field to json tag files allowing removals from existing contents.

{
  "replace": false,
  "c:remove": [
    "brick"
  ],
  "values": [
    "brick",
    "snowball"
  ]
}

[Earthcomputer]

Not too keen on this implementation, as is breaks assumptions that other mods may use for the TagEntry class. Instead, maybe TagFile should maintain a separate list for c:remove, in a way that is closer to the actual file representation.

[cputnam-a11y]

The reason it's done like this is to avoid adding a field to TagFile. We'll have to discuss alternatives. I don't really like extending TagEntry, but maybe that is preferable. I considered doing more patching to fake the field better, but decided against it for now.

[modmuss50]

Whats the concern with adding a Field to TagFile? is it the fact its a mixin? Its not a deal breaker if we are careful.

[modmuss50]

I havent done an indepth review of the impl just yet. Overall I am happy with the propsoal.

I think we should start by having this as fabric:remove. As I said on discord I am hesitant to merge a c:remove without first being 100% sure that neo are also going to adopt this, we have been burnt by this before. Lets concenrate on ourselves first as we can easily add support for c:remove later when the stars have all aligned.

Data generation support is also required for this, and I think there is a lot of scope to improve the tests.

This looks like a great start though 👍

[modmuss50]

FabricTagEntryImpl but it doesnt implement FabricTagEntry?

[cputnam-a11y]

I'm not sure what to call it, as it just contains the mixin context thread local and the codec we merge. It can implement FabricTagEntry, but then the mixin doesn't implement FabricTagEntryImpl, and if we make it an interface so the mixin can implement it, then we lose encapsulation

[cputnam-a11y]

Perhaps FabricTagEntryInternals is clearer to what it actually does?

[modmuss50]

Whats the concern with adding a Field to TagFile? is it the fact its a mixin? Its not a deal breaker if we are careful.

[modmuss50]

Nit: Static import TagTestUtils, will make the diff a little nicer.

[cputnam-a11y]

Re:

Whats the concern with adding a Field to TagFile? is it the fact its a mixin? Its not a deal breaker if we are careful.

It is a record, and there currently isn't a good way to add a record field with a mixin. With current jvm behavior it wouldn't be the end of the world, but I'd prefer to avoid it if possible. I have some unpushed commits that help with this a little, I think, so I'll re request review when I push.

Git didn't give me the option to respond to the actual comment for some reason