Deleted attributes aren't remove completely from XML about recheck HOT 1 OPEN

When working on #750 where I included special handling for inserted attributes, I took a look at how deletion works. For reference, I will include that here, too.

How it works

A deletion is represented as follows:

key: id, expected=foo, actual=(default or absent)

recheck assumes that actual=null indicates that the attribute has been deleted.

When applying an (identifying) attribute difference, it is passed down to the respective attributes collection (IdentifyingAttributes or Attributes) and eventually to the attribute itself.

The Attributes collection already handles deletions correctly, but the IdentifyingAttributes does not. It simply applies the attribute, which returns Attribute(key: id, value: null) which then results in an empty attribute in the Golden Master.

Proposal

One could assume that we simply remove that attribute from the collection. However, this is not the case, since some attributes may require special handling (e.g. the path attribute should never be removed, see IdentifyingAttributesTest).

With that in mind, I would propose either of the following:

1. The Attribute class still handles deletions as a form of change (indicated by the above PR). Thus the Attribute decides if the new value is valid and if null should result in a deletion. This can be either indicated by returning either a null attribute or an Optional<Attribute> within Attribute#applyChanges.

   This gives the power to the attribute and can be expanded. But this requires implementation of such (e.g. return the null attribute, if actual=null). Consequently, this removes power from the IdentifyingAttributes.

2. The IdentifyingAttributes handles deletions (as indicated by actual=null), but excludes special attributes.

   This gives central power to the IdentifyingAttributes to ensure the removal works for ever attribute. However, this comes at the cost, that the IdentifyingAttributes knows of the special attributes (i.e. path never be null), either by class or by key, and cannot be expanded easily.

The first approach is based on the assumption that special handling is required (currently only path) and might not even be the case anymore. I think, that this special handling is the responsibility of the extension at hand, not of the user applying the change. Thus these cases should be handled earlier and should not be reported to the user on apply.

The second approach is more clean, as the collection can decide when to remove a attribute., thus I prefer this approach. For the special handling is the decision to be made, if it should be re-implemented or simply removed for now. Thus an extension has to respect and enforce this.

from recheck.