robotools / fontParts

The replacement for RoboFab
MIT License
135 stars 44 forks source link

Discrepancy in native object identifier return #748

Open knutnergaard opened 4 weeks ago

knutnergaard commented 4 weeks ago

base.IdentifierMixin.identifier is a read-only attribute which returns str or None. The docs for the _get_identifier method states that an identifier should be assigned to any object that does not have one. IOW, it should always return str. This does not, however, seem to comply with the base getter:

def _get_base_identifier(self) -> Optional[str]:
        value = self._get_identifier()
        if value is not None:
            value = normalizers.normalizeIdentifier(value)
        return value

Given that the attribute is a mandatory override, should the documentation clarify that the _get_identifier return may be either str or None or am I missing something with regard to the logic here?

benkiel commented 3 weeks ago

iirc, None is there for reasons (@typemytype or @typesupply can shed light), so it should clarify that it can return either. Thank you!

typesupply commented 3 weeks ago

Right, if the identifier hasn't been defined None is returned. If it has been defined, it is passed through the normalizer. We do this because if this property created an identifier if one isn't defined, all objects in all UFOs would have identifiers after their next save. That would do bad things to version control and file sizes.

benkiel commented 3 weeks ago

To sum, docs should be clear that _get_identifer may return str or None