Refine craft data tag system

[oh-noey]

Describe in detail what your pull request accomplishes

This PR aims to make improvements to the new CraftDataTagContainer system in several areas. While there are API name changes, external consumers should experience no change in behavior under the conditions that were previously required for correct API usage.

• Support concurrency
  • The existing implementation does not support concurrent writes to tags, and does not support concurrent registrations
  • The new system is based on ConcurrentHashMap instances, and has proper synchronization to yield improved errors
  • The new system does not surface any additional conflict resolution support, but will allow it to be built out in the future
• Reduce internal behavior exposure
  • Favor composition over inheritance for the container object to reduce exposed API surface and allow safely tunning parameters over time
  • Avoid exposing CraftDataTagContainer from Craft via getter
  • Move system implementation to BaseCraft
  • Do not expose the REGISTERED_TAGS registry Map instance
• Refine error behavior
  • Most locations that return null in the case of an error state now throw instead. These states in general are likely to be non-recoverable due to requiring failures during program initialization, i.e. they all required invalid tag registrations
  • Added nullability annotations where applicable
• Improved documentation
  • Updated method names to match method intent (including changes in intent)
  • Added javadoc to CraftDataTagContainer

Checklist

• Unit tests
• Proper internationalization
• Tested
• Performance tested

[DerToaster98]

Why exactly did you make the „registry“ private? I made it public on purpose cause i need it for some stuff i have planned.
Implementing it via composition is a oversight on my end, good change.

But why were the implementations moved to BaseCraft? Default implementations where used on purpose to avoid exactly that cause baseCraft is already pretty cluttered.

[oh-noey]

Why exactly did you make the „registry“ private? I made it public on purpose cause i need it for some stuff i have planned.

Directly exposing the registry bypasses type validation on the key/value pairs, which allows the system to get into a bad state. If you need direct access to the registry, it would probably be better to convert the registry to a discreet object that has its own lifecycle/methods that continue to abide by the contracts on the system. I'm totally willing to do so in this PR if you can describe your scenario.

But why were the implementations moved to BaseCraft? Default implementations where used on purpose to avoid exactly that cause baseCraft is already pretty cluttered.

Having the implementation in Craft complicates the system by requiring it to handle when the container is incorrectly initialized. in reality, these scenarios can never happen, since BaseCraft is the only direct ancestor of Craft. Thus, by moving the implementation to BaseCraft, we get a cleaner API calling pattern for all users. Craft is not meant for buisness logic - it's really just a thin layer to avoid exposing logic that's internal to Movecraft when it's not needed.

[DerToaster98]

Why exactly did you make the „registry“ private? I made it public on purpose cause i need it for some stuff i have planned.

Directly exposing the registry bypasses type validation on the key/value pairs, which allows the system to get into a bad state. If you need direct access to the registry, it would probably be better to convert the registry to a discreet object that has its own lifecycle/methods that continue to abide by the contracts on the system. I'm totally willing to do so in this PR if you can describe your scenario.

But why were the implementations moved to BaseCraft? Default implementations where used on purpose to avoid exactly that cause baseCraft is already pretty cluttered.

Having the implementation in Craft complicates the system by requiring it to handle when the container is incorrectly initialized. in reality, these scenarios can never happen, since BaseCraft is the only direct ancestor of Craft. Thus, by moving the implementation to BaseCraft, we get a cleaner API calling pattern for all users. Craft is not meant for buisness logic - it's really just a thin layer to avoid exposing logic that's internal to Movecraft when it's not needed.

Hm, true, didn't think about the first point. Converting it to a separate proper registry object would be better eventually.

On the latter i am not really a fan of classes like BaseCraft, they tend to get and be way too cluttered with all sorts of nonsense, but alright. Having the implementations in the interface doesn't really change that thin exposition layer though.

[TylerS1066]

On the latter i am not really a fan of classes like BaseCraft, they tend to get and be way too cluttered with all sorts of nonsense

Part of my hope is that we can translate most of the stuff in BaseCraft to separate feature classes utilizing the craft data tag system, which should clean things up. Then Craft is the interface containing the keys, BaseCraft has the internal business logic of managing a craft instance, then the separate feature classes or packages contain all business logic for each feature.

[oh-noey]

Part of my hope is that we can translate most of the stuff in BaseCraft to separate feature classes utilizing the craft data tag system, which should clean things up. Then Craft is the interface containing the keys, BaseCraft has the internal business logic of managing a craft instance, then the separate feature classes or packages contain all business logic for each feature.

You'll never guess how I started doing this (ignore the branch name)

[DerToaster98]

Thansk for making it more accessible again, i think it would also be beneficial to add a method that gives you a copy of all registered keys as a list.