Create cross-format StandardProperties class

[NebelNidas]

Reduces redundancy and allows API consumers to have direct access to all standard properties.

[NebelNidas]

Hmm, should I rename to class to TinyFormatStandardProperties?

[liach]

This design is bad: malicious actors can fake an id and pollute the metadata detection. You should just keep a Map<MappingFormat, Map<String, StandardProperty>> instead.

[NebelNidas]

Not sure what you mean, the internal maps are all private anyway

[liach]

Say if you have a next-intermediary-method read from tiny v1, you would incorrectly interpret it as an intermediary counter while it isn't.

[NebelNidas]

I've already added a sanity check preventing this: 5341ecf

[NebelNidas]

The issue why I need a simple String ID instead of a Map<MappingFormat, Map<String, StandardProperty>> is intermediate operations, say MemoryMappingTree.accept(MemoryMappingTree), where none of the participants know which mapping format the former tree was originally constructed from. I could generate a random ID per session, but IMO that's overcomplicating things without much reason. Maybe prepending the existing ID with mio: would do?

[liach]

Can we just add a new default void visitStandardProperty(StandardProperty prop, String value) in MappingVisitor and FlatMappingVisitor, which fabric's builtin visitors will override? And we can choose to feed these standard properties back as String key-values in visit options.

[NebelNidas]

Hmm, not a huge fan of this, since up until this point the visitor- and tree-based APIs have always been cleanly separated.
Edit: On the other hand, StandardProperty isn't really part of the tree-api, so maybe it'd work? But it still duplicates the metadata visit methods, which pollute the FlatMappingVisitor interface, especially considering #41 will add even more.

[NebelNidas]

Oops, I wanted to request the review from Modmuss, sorry 😅

[modmuss50]

Im not sure these should exist in mio, they are specific to intermediary and are generated by matcher. They arent part of the tiny format, and I dont think we actually read them anymore?

[modmuss50]

happy to discuss this if you think otherwise.

[NebelNidas]

I'd say this is a bit of an edge-case, since the counters have been special-cased until now and consequently behaved as de-facto standard properties. I could remove it, so we end up using the INTERMEDIARY_COUNTER notation in Tiny v2, too, though this would conflict with existing naming conventions and we would be stuck with a misleading name. We could of course also delegate this responsibility to Matcher, which would have to do the conversion via a custom forwarding visitor pass

[NebelNidas]

In the long run we might need an API letting consumers register their own standard properties

[liach]

Also for preventing clash of property id with mapping's native properties, should we declare a new MappingFlag to alert mapping visitors? Also to preserve backward compatibility if say, an old tinyv1 visitor still anticipates old property keys instead of the standardized ones. If a visitor anticipates standardized keys, they should declare the new flag in their mapping flags.

Otherwise this whole thing looks pretty good, a huge improvement from the initial revision.

[NebelNidas]

to preserve backward compatibility if say, an old tinyv1 visitor still anticipates old property keys instead of the standardized ones. If a visitor anticipates standardized keys, they should declare the new flag in their mapping flags.

That's the nice thing, MIO already used the same standardized values internally before, so this whole PR is 100% backward compatible :)

[NebelNidas]

Postponing to a later release, since I need to rewrite this to be more extensible