Check Object documentation for Font

Be sure that style is right and that all pieces are documented as things have been added since the documentation was written.

I'd like to help with this and other parts of the documentation if time allows, but I have a few questions and proposals that will need clarification beforehand:

Is it desirable to implement type annotation, both for the benefit of documenting types and static type checking?
Is the docstring style set in stone? I'd like to alter it slightly, if I may, just to make sure it's both in line with PEP 257 standards and as consistent as possible. Rewriting all summary lines, including properties and classes, in imperative mood is one way to uphold such consistency. I also have a personal preference towards keeping the summary line and the opening quotes on the same (first) line, as well as having a blank line before the closing quotes, which I would like to employ, as I think this looks better, separating the docstring from the rest of the code more clearly.
I'd also like to employ cross references of objects, linking to other parts of the FontParts docs as well as the Python or Defcon docs where appropriate.
I'd also like to replace all markup of parameter names set in strong emphasis (boldface) with the Sphinx default role (single backsticks), which is determined by the html theme by default, but may be set to any markup style or role in conf.py.

Apart from the docs, I've noticed that the FontParts error messages are less pythonic and clear than I would prefer. For this reason, I'd like to revise those as well. I can submit a separate proposal for this with a short style guide if needed.

Hey @knutnergaard, your help with documentation is very much appreciated, yes please!

As for your questions:

Ok with 2, 3, and 4. I'm a little hesitant on 1: it indroduces some overhead but I'm not opposed. @typesupply do you think that types will end up screwing anything up?

Hi @knutnergaard, wow, thanks! This would be extremely welcome!

Is it desirable to implement type annotation, both for the benefit of documenting types and static type checking?

We have discussed this without reaching a conclusion. @typemytype likes them, while I am wondering if they are worth the effort. My main concern is backwards compatibility. I don't know much about how type annotations work in practice, so we'd need to do some viability tests. One of my biggest questions is this: if the base class has type annotations and a subclass overrides a method, does the subclass have to include the type annotations? The FontParts objects are meant to be subclassed and there are subclasses out there that are not under our control.

Is the docstring style set in stone?

No. It's mostly just my preffered typographic formatting. 😄 I like things open and easy to read.

I'd like to alter it slightly, if I may, just to make sure it's both in line with PEP 257 standards and as consistent as possible. Rewriting all summary lines, including properties and classes, in imperative mood is one way to uphold such consistency. I also have a personal preference towards keeping the summary line and the opening quotes on the same (first) line, as well as having a blank line before the closing quotes, which I would like to employ, as I think this looks better, separating the docstring from the rest of the code more clearly.

I'm not a fan of putting the first line on the same line as the quotes, but I can live with it if 257 suggests it. As for the blank line at the end, is that a 257 thing? I'm always working in an editor where I have syntax coloring so I haven't run into a problem with the doc and code looking similar.

I'd also like to employ cross references of objects, linking to other parts of the FontParts docs as well as the Python or Defcon docs where appropriate.

This would be fantastic.

I'd also like to replace all markup of parameter names set in strong emphasis (boldface) with the Sphinx default role (single backsticks), which is determined by the html theme by default, but may be set to any markup style or role in conf.py.

This would be great.

One thing: Will you keep it in Markdownn? ReST is fine, but every time I have to write it I have to pull up the documentation. I think Markdown is much easier for people who don't work with ReST regularly.

Apart from the docs, I've noticed that the FontParts error messages are less pythonic and clear than I would prefer. For this reason, I'd like to revise those as well. I can submit a separate proposal for this with a short style guide if needed.

Could you give some examples? I'm sure you are right, but I don't remember what my structure was in FontParts.

Thanks again!

Is it desirable to implement type annotation, both for the benefit of documenting types and static type checking?

We have discussed this without reaching a conclusion. @typemytype likes them, while I am wondering if they are worth the effort. My main concern is backwards compatibility. I don't know much about how type annotations work in practice, so we'd need to do some viability tests. One of my biggest questions is this: if the base class has type annotations and a subclass overrides a method, does the subclass have to include the type annotations? The FontParts objects are meant to be subclassed and there are subclasses out there that are not under our control.

The way I see it, type documentation is really important, especially when the codebase is intended for subclassing and expansion by third parties, like with FontParts. Type annotation is just a better way to document type than declaring them within the docstring (something which FontParts also doesn't do at present). In my view, type should be documented in the docstrings at a minimum.

I'm not aware of type annotations introducing any restrictions at all on type declaration of subclasses. Any type errors originating from externally imported modules can be ignored in static type checkers like Mypy, and for anyone using them when writing subclasses, type annotations provide an extra source of reliability and consistency. That said, I'm totally fine with providing annotations for an initial module and converting them to docstring declarations if necessary.

I'm also not aware of type annotation introducing any overhead at runtime, unless actually featuring them in the logic, but I haven't tested this myself.

Is the docstring style set in stone?

No. It's mostly just my preffered typographic formatting. 😄 I like things open and easy to read.

I see. :)

I'd like to alter it slightly, if I may, just to make sure it's both in line with PEP 257 standards and as consistent as possible. Rewriting all summary lines, including properties and classes, in imperative mood is one way to uphold such consistency. I also have a personal preference towards keeping the summary line and the opening quotes on the same (first) line, as well as having a blank line before the closing quotes, which I would like to employ, as I think this looks better, separating the docstring from the rest of the code more clearly.

I'm not a fan of putting the first line on the same line as the quotes, but I can live with it if 257 suggests it. As for the blank line at the end, is that a 257 thing? I'm always working in an editor where I have syntax coloring so I haven't run into a problem with the doc and code looking similar.

PEP257 leaves this aspect up to the user, but it's part of the numpydoc style guide and I personally prefer it over the FontParts style, even though that is also fine, according to the standard.

I'd also like to replace all markup of parameter names set in strong emphasis (boldface) with the Sphinx default role (single backsticks), which is determined by the html theme by default, but may be set to any markup style or role in conf.py.

This would be great.

One thing: Will you keep it in Markdownn? ReST is fine, but every time I have to write it I have to pull up the documentation. I think Markdown is much easier for people who don't work with ReST regularly.

As far as I can tell from the documentation files and docstrings, all the documentation is written in ReST already. Perhaps I'm misunderstanding your question, but my intention was to keep in as reStructuredText. I agree that Mardown is easier to read, but ReST is much more powerful, in my experience.

BTW, most, if not all docstrings in FontParts currently seem to lack systematic parameter and exception declarations (which may be a Markdown thing?) I much prefer the structured way in which ReST handles this over the verbose style FontParts currently employs, even if the syntax takes some getting used to. This is another thing I'd like to change, which I didn't initially mention.

Apart from the docs, I've noticed that the FontParts error messages are less pythonic and clear than I would prefer. For this reason, I'd like to revise those as well. I can submit a separate proposal for this with a short style guide if needed.

Could you give some examples? I'm sure you are right, but I don't remember what my structure was in FontParts.

Sure! In general errors in FontParts are usually formulated along the lines of these examples:

"Kerning key must be a tuple instance, not list."
"Kerning key items must be strings, not int."
"Left Kerning key group must start with public.kern1."

I'd like to stick to the actual pythonic type names, and consistently keep class names or any other specific object, parameter or value references in quotes to distinguish it from the rest of the text. Generally, I also prefer not to start the message with a reference. With that in mind, I'd reformulate the messages in a typical pythonic fashion as follows:

"Expected 'Kerning' key to be of type tuple, but got list."
"Expected 'Kerning' key items to be of type str, but got int."
"Left 'Kerning' key group must start with 'public.kern1'."

I also prefer to return any invalid input value whenever possible, so a variation of the last example, where the input value is practically possible to collect, would typically read:

"Left 'Kerning' key group must start with 'public.kern1', not 'inputValue'."

For general value errors like this:

"Invalid value inputValue for attribute 'attributeName'."

I think a colon before the input value and quotes around both object/attribute name and input value is more typical and preferable:

"Invalid value for attribute 'attributeName': 'inputValue'."

Special cases will have to be discussed

I'm not aware of type annotations introducing any restrictions at all on type declaration of subclasses.

Great. I just made a test with some bogus data to see if it would break and it doesn't. This is the first time I've tried type annotations, so there's a good chance I did something wrong.

class Base:

    def foo(self, bar: float = 0.00001) -> int:
        print(self.__class__, "bar", bar)
        return int(round(bar))

class Test(Base):

    def foo(self, bar=123456):
        return super().foo(bar)

t = Test()
b = t.foo(0.987)
print(b)

So, if you want to do the annotations, I have no objection.

PEP257 leaves this aspect up to the user, but it's part of the numpydoc style guide and I personally prefer it over the FontParts style, even though that is also fine, according to the standard.

If you want the space, go for it. I can live with it for all of these other improvements.

As far as I can tell from the documentation files and docstrings, all the documentation is written in ReST already.

Ha. I have written so much code that I forgot. I switched to Markdown at some point after FontParts.

BTW, most, if not all docstrings in FontParts currently seem to lack systematic parameter and exception declarations (which may be a Markdown thing?)

It's probably a "Tal was working very fast and was very tired." thing. Feel free to fix it.

Sure!

The error messages you propose look good to me. My only note is to keep in mind that almost all of the users of FontParts are designers writing little scripts. The more clear messages can be about how to fix the problem, the better.

Thanks again!

I'm not aware of type annotations introducing any restrictions at all on type declaration of subclasses.

Great. I just made a test with some bogus data to see if it would break and it doesn't. This is the first time I've tried type annotations, so there's a good chance I did something wrong.
class Base:

    def foo(self, bar: float = 0.00001) -> int:
        print(self.__class__, "bar", bar)
        return int(round(bar))

class Test(Base):

    def foo(self, bar=123456):
        return super().foo(bar)

t = Test()
b = t.foo(0.987)
print(b)
So, if you want to do the annotations, I have no objection.

Great! If the requirements stated in setup.py (Python 3.7) is right, I think they will be a welcome addition for both users and developers. However, if support for python 3.6 is required, as stated in the readme, they might not be worth the trouble. Type annotations are greatly simplified and streamlined from Python 3.10 onward. Prior to that, they are a bit cluttery in complex cases. Anyway, I've done them as an example for the first half of the font.py module (see below), just to see how you like it if nothing else.

PEP257 leaves this aspect up to the user, but it's part of the numpydoc style guide and I personally prefer it over the FontParts style, even though that is also fine, according to the standard.

If you want the space, go for it. I can live with it for all of these other improvements.

Excellent!

As far as I can tell from the documentation files and docstrings, all the documentation is written in ReST already.

Ha. I have written so much code that I forgot. I switched to Markdown at some point after FontParts.

A-ha!

BTW, most, if not all docstrings in FontParts currently seem to lack systematic parameter and exception declarations (which may be a Markdown thing?)

It's probably a "Tal was working very fast and was very tired." thing. Feel free to fix it.

Will do!

Sure!

The error messages you propose look good to me. My only note is to keep in mind that almost all of the users of FontParts are designers writing little scripts. The more clear messages can be about how to fix the problem, the better.

I totally agree with that sentiment, which is why I feel at least some of the messages need editing.

As mentioned above, I've added type annotations and edited the documentation for the first half of font.py as an example:

from __future__ import annotations
import os
from typing import (
    TYPE_CHECKING, Any, Dict, Generic, List, Optional, Tuple, TypeVar, Union
)

from fontTools import ufoLib
from fontParts.base.errors import FontPartsError
from fontParts.base.base import dynamicProperty, InterpolationMixin
from fontParts.base.layer import _BaseGlyphVendor
from fontParts.base import normalizers
from fontParts.base.compatibility import FontCompatibilityReporter
from fontParts.base.deprecated import DeprecatedFont, RemovedFont

if TYPE_CHECKING:
    from fontParts.base.info import BaseInfo
    from fontParts.base.groups import BaseGroups
    from fontParts.base.kerning import BaseKerning
    from fontParts.base.features import BaseFeatures
    from fontParts.base.lib import BaseLib

FontType = TypeVar('FontType', bound='BaseFont')
InfoType = TypeVar('InfoType', bound='BaseInfo')
GroupsType = TypeVar('GroupsType', bound='BaseGroups')
KerningType = TypeVar('KerningType', bound='BaseKerning')
FeaturesType = TypeVar('FeaturesType', bound='BaseFeatures')
LibType = TypeVar('LibType', bound='BaseLib')
KerningKey = Tuple[str, str]
KerningValue = Union[int, float]
KerningDict = Dict[KerningKey, KerningValue]

class BaseFont(_BaseGlyphVendor,
               InterpolationMixin,
               DeprecatedFont,
               RemovedFont,
               Generic[FontType]):
    """Base class representing a font object.

    Instances of this class are almost always created with one of the
    font functions in :ref:`fontparts-world`.

    """

    def __init__(self,
                 pathOrObject: Optional[Union[str, 'BaseFont']] = None,
                 showInterface: bool = True) -> None:
        """
        This class will be instantiated in different ways, depending on
        the value type of the `pathOrObject` parameter.

        :param pathOrObject: The source for initializing the font.
            If :obj:`None`, a new, empty font will be created. If
            a :class:`str` representing the path to an existing file,
            the class will open and read the file at this path. If an
            instance of the environment's unwrapped native font object,
            it will be wrapped with FontParts. Defaults to :obj:`None`.
        :param showInterface: Whether to display the graphical
            interface. Defaults to :obj:`True`.

        """
        super(BaseFont, self).__init__(pathOrObject=pathOrObject,
                                       showInterface=showInterface)

    def _reprContents(self) -> List[str]:
        contents = [
            "'%s %s'" % (self.info.familyName, self.info.styleName),
        ]
        if self.path is not None:
            contents.append("path=%r" % self.path)
        return contents

    # ----
    # Copy
    # ----

    copyAttributes: Tuple[str, ...] = (
        "info",
        "groups",
        "kerning",
        "features",
        "lib",
        "layerOrder",
        "defaultLayerName",
        "glyphOrder"
    )

    def copy(self) -> BaseFont:
        """Copy this font into a new font.

        This will copy:

            - info
            - groups
            - kerning
            - features
            - lib
            - layers
            - layerOrder
            - defaultLayerName
            - glyphOrder
            - guidelines

        :return: A new font instance with the same attributes.

        Example::

            >>> copiedFont = font.copy()

        """
        return super(BaseFont, self).copy()

    def copyData(self, source: BaseFont) -> None:
        """Copy data from `source` into this font.

        Refer to :meth:`BaseFont.copy` for a list of values that will be
        copied.

        :param source: The source font instance from which to copy
         data.

        Example::

            >>> sourceFont = MyFont('path/to/source.ufo')
            >>> font.copyData(sourceFont)

        """
        # set the default layer name
        self.defaultLayer.name = source.defaultLayerName
        for layerName in source.layerOrder:
            if layerName in self.layerOrder:
                layer = self.getLayer(layerName)
            else:
                layer = self.newLayer(layerName)
            layer.copyData(source.getLayer(layerName))
        for guideline in self.guidelines:
            self.appendGuideline(guideline)
        super(BaseFont, self).copyData(source)

    # ---------------
    # File Operations
    # ---------------

    # Initialize

    def _init(self,
              pathOrObject: Optional[Union[str, FontType]] = None,
              showInterface: bool = True,
              **kwargs: Any) -> None:
        r"""Initialize the native font object.

        This method is the environment implementation
        of :meth:`BaseFont.__init__`. It should wrap a native font
        object based on the value type of the `pathOrObject` parameter.

        .. important::

            Subclasses must override this method.

        :param pathOrObject: The source for initializing the font.
            If :obj:`None`, a new, empty font is created. If
            a :class:`str`, the method opens the font file located at
            the given path. If a native font object, it wraps the
            provided object. Defaults to :obj:`None`.
        :param showInterface: Whether to display the graphical
            interface. Defaults to :obj:`True`.
        :param \**kwargs: Additional keyword arguments.
        :raises NotImplementedError: If the method has not been
            overridden by a subclass.

        """
        self.raiseNotImplementedError()

    # path

    path = dynamicProperty(
        "base_path",
        """Get the path to the font file.

        Example::

            >>> print(font.path)
            "/path/to/my/font.ufo"

        """
    )

    def _get_base_path(self: BaseFont) -> str:
        path = self._get_path()
        if path is not None:
            path = normalizers.normalizeFilePath(path)
        return path

    def _get_path(self, **kwargs: Any) -> str:
        r"""Get the path to the native font file.

        This method is the environment implementation
        of :attr:`BaseFont.path`.

        .. important::

            Subclasses must override this method.

        :param \**kwargs: Additional keyword arguments.
        :return: A :class:`str` defining the location of the file
            or :obj:`None` to indicate that the font does not have a
            file representation. If the value is not :obj:`None` it will
            be normalized with :func:`normalizers.normalizeFilePath`.
        :raises NotImplementedError: If the method has not been
            overridden by a subclass.

        """
        self.raiseNotImplementedError()

    # save

    def save(self,
             path: Optional[str] = None,
             showProgress: bool = False,
             formatVersion: Optional[int] = None,
             fileStructure: Optional[str] = None) -> None:
        """Save the font to the specified path.

        :param path: The path to which the font should be saved.
            If :obj:`None`, the font is saved to its original location.
            The file type is inferred from the file extension of the
            path. If no extension is given, the environment may use a
            default format.
        :param showProgress: Whether to display a progress bar during
            the operation. Environments may or may not implement this
            behavior.
        :param formatVersion: The format version to use when saving the
            file. For example, a `formatVersion` of 2 will save the file
            in UFO 2 format. If :obj:`None`, the original format version
            will be preserved, or the latest version supported by the
            environment will be used if no original version exists.
        :param fileStructure: The file structure for saving UFO formats.
            Can be :obj:`None`, which uses the existing file structure
            or the default structure for unsaved fonts; ``'package'``,
            which is the default structure; or ``'zip'``, which saves
            the font as a ``.ufoz`` file.

        .. note::

            Environments may define their own rules regarding when a
            file should be saved to its original location versus a new
            location. For example, a font opened from a compiled
            OpenType font may not be saved back into the original
            OpenType file.

        Example::

            >>> font.save()
            >>> font.save("/path/to/my/font-2.ufo")

        """
        if path is None and self.path is None:
            raise IOError(("The font cannot be saved because no file "
                           "location has been given."))
        if path is not None:
            path = normalizers.normalizeFilePath(path)
        showProgress = bool(showProgress)
        if formatVersion is not None:
            formatVersion = normalizers.normalizeFileFormatVersion(
                formatVersion)
        if fileStructure is not None:
            fileStructure = normalizers.normalizeFileStructure(fileStructure)
        self._save(path=path, showProgress=showProgress,
                   formatVersion=formatVersion, fileStructure=fileStructure)

    def _save(self,
              path: Optional[str] = None,
              showProgress: bool = False,
              formatVersion: Optional[float] = None,
              fileStructure: Optional[str] = None,
              **kwargs: Any) -> None:
        r"""Save the native font to the specified path.

        This is the environment implementation of :meth:`BaseFont.save`.

        .. important::

            Subclasses must override this method.

        :param path: The file path to save the data to. If
            not :obj:`None`, the value will be normalized
            with :func:`normalizers.normalizeFilePath`.
        :param showProgress: Whether to display a progress bar during
            the operation. Environments are not required to display a
            progress bar even if value is :obj:`True`.
        :param formatVersion: The file format version to write the data
            into.  If not :obj:`None`, the value will be normalized
            with :func:`normalizers.normalizeFileFormatVersion`.
        :param fileStructure: The file structure to use.
        :param \**kwargs: Additional keyword arguments.
        :raises NotImplementedError: If the method has not been
            overridden by a subclass.

        """
        self.raiseNotImplementedError()

    # close

    def close(self, save: bool = False) -> None:
        """Close the font.

        :param save: Whether to save the font before closing.

        Example::

            >>> font.close()
            >>> font.close(save=True)

        """
        if save:
            self.save()
        self._close()

    def _close(self, **kwargs: Any) -> None:
        r"""Close the native font.

        This is the environment implementation
        of :meth:`BaseFont.close`.

        .. important::

            Subclasses must override this method.

        :param \**kwargs: Additional keyword arguments.
        :raises NotImplementedError: If the method has not been
            overridden by a subclass.

        """
        self.raiseNotImplementedError()

    # generate

    @staticmethod
    def generateFormatToExtension(format: str, fallbackFormat: str) -> str:
        """Return the file extension for the given format identifier.

        This method maps format identifiers to file extensions. If the
        provided `format` is not in the map, the method returns the
        `fallbackFormat`.

        Format Identifiers:

        +------------+-------------------------------------------------------+
        | Format     | Description                                           |
        +------------+-------------------------------------------------------+
        | macttf     | Mac TrueType font (generates suitcase)                |
        | macttdfont | Mac TrueType font (generates suitcase with data fork) |
        | otfcff     | PS OpenType (CFF-based) font (OTF)                    |
        | otfttf     | PC TrueType/TT OpenType font (TTF)                    |
        | ufo1       | UFO format version 1                                  |
        | ufo2       | UFO format version 2                                  |
        | ufo3       | UFO format version 3                                  |
        | unixascii  | UNIX ASCII font (ASCII/PFA)                           |
        +------------+-------------------------------------------------------+

        :param format: The format identifier to map to a file extension.
        :param fallbackFormat: The extension to return if `format` is
            unrecognized.
        :return: The corresponding file extension for the `format`
            identifier or the `fallbackFormat` if the format is
            unrecognized.

        """
        formatToExtension = dict(
            # mactype1=None,
            macttf=".ttf",
            macttdfont=".dfont",
            otfcff=".otf",
            otfttf=".ttf",
            # pctype1=None,
            # pcmm=None,
            # pctype1ascii=None,
            # pcmmascii=None,
            ufo1=".ufo",
            ufo2=".ufo",
            ufo3=".ufo",
            unixascii=".pfa",
        )
        return formatToExtension.get(format, fallbackFormat)

    def generate(self,
                 format: str,
                 path: Optional[str] = None,
                 **environmentOptions: Any) -> None:
        r"""Generate the font in another format.

        This method converts the font to the specified format and saves
        it to the specified path. Standard format identifiers can be
        found in :attr:`BaseFont.generateFormatToExtension`.

        Environments may support additional keyword arguments in this
        method. For example, if the tool allows decomposing components
        during generation, this can be specified with an additional
        keyword argument.

        Example::

            >>> font.generate("otfcff")
            >>> font.generate("otfcff", "/path/to/my/font.otf")

        :param format: The file format identifier for the output.
        :param path: The location to save the generated file. If not
            provided, the file will be saved in the same directory as
            the source font, with the current file name and the
            appropriate suffix for the format. If a directory is
            specified, the file will be saved with the current file
            name and the appropriate suffix for the format. If a file
            already exists at that location, it will be overwritten.
        :param \**environmentOptions: Additional keyword arguments for
            environment-specific options.
        :raises ValueError: If `format` is not defined.
        :raises TypeError: If `format` is not a string.
        :raises UserWarning: If an unsupported environment option is
            passed.
        :raises IOError: If the output path is not defined and the
            source font does not have a path.

        """
        import warnings
        if format is None:
            raise ValueError("The format must be defined when generating.")
        elif not isinstance(format, str):
            raise TypeError("The format must be defined as a string.")
        env = {}
        for key, value in environmentOptions.items():
            valid = self._isValidGenerateEnvironmentOption(key)
            if not valid:
                warnings.warn("The %s argument is not supported "
                              "in this environment." % key, UserWarning)
            env[key] = value
        environmentOptions = env
        ext = self.generateFormatToExtension(format, "." + format)
        if path is None and self.path is None:
            raise IOError(("The file cannot be generated because an "
                           "output path was not defined."))
        elif path is None:
            path = os.path.splitext(self.path)[0]
            path += ext
        elif os.path.isdir(path):
            if self.path is None:
                raise IOError(("The file cannot be generated because "
                               "the file does not have a path."))
            fileName = os.path.basename(self.path)
            fileName += ext
            path = os.path.join(path, fileName)
        path = normalizers.normalizeFilePath(path)
        return self._generate(
            format=format,
            path=path,
            environmentOptions=environmentOptions
        )

    @staticmethod
    def _isValidGenerateEnvironmentOption(name: str) -> bool:
        """Validate if the environment option is supported.

        Any unknown keyword arguments given to :meth:`BaseFont.generate`
        are passed to this method. `name` is the name used for the
        argument. Environments may evaluate if `name` is a supported
        option.

        .. note::

            Subclasses may override this method.

        :param name: The name of the environment option to validate.
        :return: :obj:`True` if the environment option is supported,
            otherwise :obj:`False`.

        """
        return False

    def _generate(self,
                  format: str,
                  path: str,
                  environmentOptions: dict,
                  **kwargs: object) -> None:
        """Generate the native font in another format.

        This is the environment implementation
        of :meth:`BaseFont.generate`. Refer to
        the :attr:`BaseFont.generateFormatToExtension` documentation
        for the standard format identifiers.

        .. important::

            Subclasses must override this method.

        :param format: The output format identifier. If the value given
            for `format` is not supported by the environment,
            the environment must raise :exc:`FontPartsError`.
        :param path: The location where the generated file should be
            saved. It is normalized
            with :func:`normalizers.normalizeFilePath`.
        :param environmentOptions: A dictionary of environment-specific
            options. These options are validated
            with :meth:`BaseFont._isValidGenerateEnvironmentOption` and
            the given values. These values are not passed through any
            normalization functions.
        :raises NotImplementedError: If the method has not been
            overridden by a subclass.

        """
        self.raiseNotImplementedError()

    # -----------
    # Sub-Objects
    # -----------

    # info

    info: BaseInfo = dynamicProperty(
        "base_info",
        """Get the font's info object.

        Example::

            >>> font.info.familyName
            "My Family"

        :return: An instance of the :class:`BaseInfo` class.

        """
    )

    def _get_base_info(self) -> BaseInfo:
        info = self._get_info()
        info.font = self
        return info

    def _get_info(self) -> InfoType:
        """Get the native font's Info object.

        This is the environment implementation of :attr:`BaseFont.info`.

        .. important::

            Subclasses must override this method.

        :return: An instance of a :class:`BaseInfo` subclass.
        :raises NotImplementedError: If the method has not been
         overridden by a subclass.

        """
        self.raiseNotImplementedError()

    # groups

    groups: BaseGroups = dynamicProperty(
        "base_groups",
        """Get the font's groups object.

        Example::

            >>> font.groups["myGroup"]
            ["A", "B", "C"]

        :return: An instance of the :class:`BaseGroups` class.

        """
    )

    def _get_base_groups(self) -> BaseGroups:
        groups = self._get_groups()
        groups.font = self
        return groups

    def _get_groups(self) -> GroupsType:
        """Get the native font's Groups object.

        This is the environment implementation
        of :attr:`BaseFont.groups`.

        .. important::

            Subclasses must override this method.

        :return: an instance of a :class:`BaseGroups` subclass.
        :raises NotImplementedError: If the method has not been
         overridden by a subclass.

        """
        self.raiseNotImplementedError()

    # kerning

    kerning: BaseKerning = dynamicProperty(
        "base_kerning",
        """Get the font's :class:`BaseKerning` object.

        Example::

            >>> font.kerning["A", "B"]
            -100

        """
    )

    def _get_base_kerning(self) -> BaseKerning:
        kerning = self._get_kerning()
        kerning.font = self
        return kerning

    def _get_kerning(self) -> KerningType:
        """Get the native font's Kerning object.

        This is the environment implementation
        of :attr:`BaseFont.kerning`.

        .. important::

            Subclasses must override this method.

        :return: An instance of a :class:`BaseKerning` subclass.
        :raises NotImplementedError: If the method has not been
         overridden by a subclass.

        """
        self.raiseNotImplementedError()

    def getFlatKerning(self) -> KerningDict:
        """Get the font's kerning as a flat dictionary.

        :return: A :class:`dict` of the font's :class:`BaseKerning` keys
            mapped to their respective values.

         """
        return self._getFlatKerning()

    def _getFlatKerning(self) -> KerningDict:
        """Get the native font's kerning as a flat dictionary.

        This is the environment implementation of
        :meth:`BaseFont.getFlatKerning`.

        .. note::

            Subclasses may override this method.

        :return: A :class:`dict` of the font's :class:`BaseKerning`
            subclass keys mapped to their respective values.

        """
        kernOrder = {
            (True, True): 0,  # group group
            (True, False): 1,  # group glyph
            (False, True): 2,  # glyph group
            (False, False): 3,  # glyph glyph
        }

        def kerningSortKeyFunc(pair):
            g1, g2 = pair
            g1grp = g1.startswith("public.kern1.")
            g2grp = g2.startswith("public.kern2.")
            return (kernOrder[g1grp, g2grp], pair)

        flatKerning = dict()
        kerning = self.kerning
        groups = self.groups

        for pair in sorted(self.kerning.keys(), key=kerningSortKeyFunc):
            kern = kerning[pair]
            (left, right) = pair
            if left.startswith("public.kern1."):
                left = groups.get(left, [])
            else:
                left = [left]

            if right.startswith("public.kern2."):
                right = groups.get(right, [])
            else:
                right = [right]

            for r in right:
                for l in left:
                    flatKerning[(l, r)] = kern

        return flatKerning

    # features

    features: BaseFeatures = dynamicProperty(
        "base_features",
        """Get the font's :class:`BaseFeatures` object.

        Example::

            >>> font.features.text
            "include(features/substitutions.fea);"

        """
    )

    def _get_base_features(self) -> BaseFeatures:
        features = self._get_features()
        features.font = self
        return features

    def _get_features(self) -> FeaturesType:
        """Get the native font's Features object.

        This is the environment implementation of
        :attr:`BaseFont.features`.

        .. important::

            Subclasses must override this method.

        :return: An instance of a :class:`BaseFeatures` subclass.
        :raises NotImplementedError: If the method has not been
            overridden by a subclass.

        """
        self.raiseNotImplementedError()

    # lib

    lib: BaseLib = dynamicProperty(
        "base_lib",
        """Get the font's :class:`BaseLib` object.

        Example::

            >>> font.lib["org.robofab.hello"]
            "world"

        """
    )

    def _get_base_lib(self) -> BaseLib:
        lib = self._get_lib()
        lib.font = self
        return lib

    def _get_lib(self) -> LibType:
        """Get the native font's Lib object.

        This is the environment implementation of :attr:`BaseFont.lib`.

        .. important::

            Subclasses must override this method.

        :return: An instance of a :class:`BaseLib` subclass.
        :raises NotImplementedError: If the method has not been
            overridden by a subclass.

        """
        self.raiseNotImplementedError()

    # tempLib

    tempLib: BaseLib = dynamicProperty(
        "base_tempLib",
        """Get the font's :class:`BaseLib` object.

        Example::

            >>> font.tempLib["org.robofab.hello"]
            "world"

        """
    )

    def _get_base_tempLib(self) -> BaseLib:
        lib = self._get_tempLib()
        lib.font = self
        return lib

    def _get_tempLib(self) -> LibType:
        """Get the native font's Lib object.

        This is the environment implementation
        of :attr:`BaseFont.tempLib`.

        .. important::

            Subclasses must override this method.

        :return: An instance of a :class:`BaseLib` subclass.
        :raises NotImplementedError: If the method has not been
            overridden by a subclass.

        """
        self.raiseNotImplementedError()

The docstring edits and additions can be summarised as a style guide as follows:

General Structure: Follow the reStructuredText (reST) format.
Line length: Follow PEP guidelines of 72 characters maximum.
Summary Line: Start with a brief summary in imperative mood, followed by a blank line.
Examples: Use the >>> prompt to show examples of usage. Examples should be preceded by the text ‘Example::’ (or ‘Examples::’ if multiple).
Notes: Use .. important:: directive to highlight mandatory implementation. Use .. note:: directive to highlight optional implementation.
Return Statement: Use :return: to describe the return value.
Parameter References: Enclose references to parameters within the docstring in single backticks.
Boolean and None References: Use the :obj: role for references to boolean values or None.
Other Object References: Use the appropriate :role: types for references (e.g., :class:, :meth:, :attr:).
Avoid :type: and :rtype: roles; use type annotations instead.
Quote placement: Opening quotes should be on the same line as the summary line (first). Closing quotes should be preceded by a blank line.

What I'm most unsure about is the summary lines of public base class methods vs. the private subclass methods. I think it's important to find a consistent and clear form for these which simultaneously follows the general style.

I'm also unsure whether the docstring for the init method should be kept as is, moved to the general class docstring or supplemented by a summary line similar to all other methods. The latter seems to be the more correct option according to PEP, but this looks odd, considering how the class and init docstrings are distributed in the theme.

I personally think the 5th point is a good addition, but the theme does not presently recognise the .. important:: directive, so this will have to be implemented if this becomes an agreed upon addition.

FontParts has it's own 'Common Value Types' which documentation of type tend to reference. In my edits, I cross-reference types with the Python docs directly. If internal references are preferable, I would suggest adding any necessary types to 'Common Value Types', and cross-reference each builtin type against the Python docs from here.

To make external references to the Python docs work, 'sphinx.ext.intersphinx' must be added to the extensions in conf.py, in addition to an inter sphinx mapping to the Python 3 docs:

intersphinx_mapping = {
    'python': ('https://docs.python.org/3/', None)
}

Lastly, I noticed that tempLib and lib (disregarding what I believe to be minor documentation errors) are just duplicate implementations with different names. I found the same temLib attribute in the defcon library, but couldn't find any documentation or discussion about it's purpose anywhere. Could you please enlighten me, so that I can clarify this in the docs?

Knut

Please forgive me for piling on to this thread. I'm just wondering more generally about the reason for the Common Value Types definitions in the documentation. Is the idea behind them that since environments implementing FontParts might not be based on Python, that common value types need to be more generically defined than the Python-specific equivalents?

If so, I think the type annotation (and object documentation) should reflect this, most likely by defining some custom type classes, e.g.:

class ImmutableList(Generic[T], Tuple[T, ...]):
    """Represent a generic, immutable list type.

    This class extends the built-in :class:`tuple` type and ensures that
    all elements are of the same specified type.

    :param T: The type of elements in the immutable list.

    """

    def __new__(cls, *args: T) -> 'ImmutableList[T]':
        """Create a new :class:`ImmutableList` instance.

        :param args: A variable number of arguments of type `T`.
        :return: An instance of :class:`ImmutableList` containing the
            provided elements.

        """
        return super().__new__(cls, args)

This way, public getters which return an actual Python tuple may be annotated:

def _get_base_layers(self) -> Tuple[BaseLayer, ...]:
        layers = self._get_layers()
        for layer in layers:
            self._setFontInLayer(layer)
        return tuple(layers)

while the private equivalent, employing an environment-specific immutable list type, may be annotated:

def _get_layers(self, **kwargs: Any) -> ImmutableList[BaseLayer, ...]:
        """Get the native font's layer objects.

        This is the environment implementation of
        :attr:`BaseFont.layers`.

        .. important::

            Subclasses must override this method.

        :return: A :ref:`type-immutable-list` containing instances
            of :class:`BaseLayer` subclasses. The items  should be in
            the order defined by :attr:`BaseFont.layerOrder`.

        """
        self.raiseNotImplementedError()

For more specific value types like strings, integers and floats, it will probably be sufficient to clarify the connection between the Python-specific types (str, int and float) used in the object documentation and environment specific equivalents in the Common Value Types section.

If not, however, I think the documentation would benefit from sticking to Python-sepecific types like tuple instead of the more generic 'immutable list' type cited in the docs.

PS. This question might perhaps tempt you to reconsider implementing type annotation in FontParts altogether. In my opinion, however, the documentation needs to clarify the connection between at least some of the common value types in FontParts and their Python equivalents, whether the code features type annotations or not.

Sorry for the delay. Things have been busy at my day job...

This all looks really good at a quick glance. @benkiel @LettError @typemytype Do you have

The docstring edits and additions can be summarised as a style guide as follows:

I agree with all of thse. Some notes on specific ones:

Line length: Follow PEP guidelines of 72 characters maximum.

I disagree with this part of the PEP, but you can stick with it if you want to. We’re not working on basic text editors on tiny screens any more and, as someone who thinks about typography almost all the time, I think this narrow column forces the text to look fractured when variable/function/method names are long. That makes it harder to read. But, I think some of the code formatters out there enforce this. I stopped using Black or whatever it was called because of this.

What I'm most unsure about is the summary lines of public base class methods vs. the private subclass methods. I think it's important to find a consistent and clear form for these which simultaneously follows the general style.

I agree. The audience for the private subclass methods is the developer community, so it can be more technically worded if needed. The primary ton focus should be on the public, scripting documentation.

I'm also unsure whether the docstring for the init method should be kept as is, moved to the general class docstring or supplemented by a summary line similar to all other methods. The latter seems to be the more correct option according to PEP, but this looks odd, considering how the class and init docstrings are distributed in the theme.

I have no opinion on this except to say that I have been wondering the same thing for years.

I personally think the 5th point is a good addition, but the theme does not presently recognise the .. important:: directive, so this will have to be implemented if this becomes an agreed upon addition.

I’m sure we can update the theme.

FontParts has it's own 'Common Value Types' which documentation of type tend to reference. In my edits, I cross-reference types with the Python docs directly. If internal references are preferable, I would suggest adding any necessary types to 'Common Value Types', and cross-reference each builtin type against the Python docs from here.

Referencing the Python documentation directly is perfect.

Lastly, I noticed that tempLib and lib (disregarding what I believe to be minor documentation errors) are just duplicate implementations with different names. I found the same temLib attribute in the defcon library, but couldn't find any documentation or discussion about it's purpose anywhere. Could you please enlighten me, so that I can clarify this in the docs?

These are separate things, but with similar functionality. The difference is that anything you put in lib will be stored in the UFO on save but anything in tempLib will not be saved. This is very useful for temporary UI and tool data that doesn’t need to be persistent from one file editing session to another. It is also useful for storing objects needed by custom tools. For example, an interpolation preview tool may add a special caching object to font.tempLib[“com.example.InterpolationTool”]. Please let me know if that makes sense. tempLib is one of the better things that we’ve added over the years. 😄

I hope this all makes sense and answers your questions. I’ll get to the next post ASAP.

Please forgive me for piling on to this thread. I'm just wondering more generally about the reason for the Common Value Types definitions in the documentation. Is the idea behind them that since environments implementing FontParts might not be based on Python, that common value types need to be more generically defined than the Python-specific equivalents?

I think my idea was that we could have documentation that was less overwhelming for scripters than the full Python documentation. I don’t necessarily agree with that idea now, so it can change to go directly to the Python docs.

Does that resolve the issue?

The docstring edits and additions can be summarised as a style guide as follows:

I agree with all of thse. Some notes on specific ones:

Line length: Follow PEP guidelines of 72 characters maximum.

I disagree with this part of the PEP, but you can stick with it if you want to. We’re not working on basic text editors on tiny screens any more and, as someone who thinks about typography almost all the time, I think this narrow column forces the text to look fractured when variable/function/method names are long. That makes it harder to read. But, I think some of the code formatters out there enforce this. I stopped using Black or whatever it was called because of this.

I see your point. It doesn't really matter to me, as long as we agree on a maximum line length. The PEP standard for code is 79 characters, which I'm used to upholding. For documentation .rst files, the standard – according to the Python Developer's Guide – is 80 characters maximum. I suggest setting the limit somewhere around there.

What I'm most unsure about is the summary lines of public base class methods vs. the private subclass methods. I think it's important to find a consistent and clear form for these which simultaneously follows the general style.

I agree. The audience for the private subclass methods is the developer community, so it can be more technically worded if needed. The primary ton focus should be on the public, scripting documentation.

Great! I take this to mean that you're OK with the summary lines in the example, at least as a starting point. I'll continue in the same vein, if no-one else objects.

I'm also unsure whether the docstring for the init method should be kept as is, moved to the general class docstring or supplemented by a summary line similar to all other methods. The latter seems to be the more correct option according to PEP, but this looks odd, considering how the class and init docstrings are distributed in the theme.

I have no opinion on this except to say that I have been wondering the same thing for years.

In that case, I would suggest integrating __init__ docstrings into their respective class docstrings. Even though this is not exactly according to the PEP, it seems to be a widely employed. It avoids splitting up crucial information about the class in the generated help(), and takes care of the problem of an extra summary line in the middle of class descriptions in the FontParts online docs.

I personally think the 5th point is a good addition, but the theme does not presently recognise the .. important:: directive, so this will have to be implemented if this becomes an agreed upon addition.

I’m sure we can update the theme.

Great!

FontParts has it's own 'Common Value Types' which documentation of type tend to reference. In my edits, I cross-reference types with the Python docs directly. If internal references are preferable, I would suggest adding any necessary types to 'Common Value Types', and cross-reference each builtin type against the Python docs from here.

Referencing the Python documentation directly is perfect.

Excellent!

Lastly, I noticed that tempLib and lib (disregarding what I believe to be minor documentation errors) are just duplicate implementations with different names. I found the same temLib attribute in the defcon library, but couldn't find any documentation or discussion about it's purpose anywhere. Could you please enlighten me, so that I can clarify this in the docs?

These are separate things, but with similar functionality. The difference is that anything you put in lib will be stored in the UFO on save but anything in tempLib will not be saved. This is very useful for temporary UI and tool data that doesn’t need to be persistent from one file editing session to another. It is also useful for storing objects needed by custom tools. For example, an interpolation preview tool may add a special caching object to font.tempLib[“com.example.InterpolationTool”]. Please let me know if that makes sense. tempLib is one of the better things that we’ve added over the years. 😄

That makes perfect sense. I'll be sure to clarify this aspect in the docs.

Please forgive me for piling on to this thread. I'm just wondering more generally about the reason for the Common Value Types definitions in the documentation. Is the idea behind them that since environments implementing FontParts might not be based on Python, that common value types need to be more generically defined than the Python-specific equivalents?

I think my idea was that we could have documentation that was less overwhelming for scripters than the full Python documentation. I don’t necessarily agree with that idea now, so it can change to go directly to the Python docs.

Does that resolve the issue?

I see. It does resolve the issue with regard to cross referencing, but not with regard to what is actually meant by "immutable list" and how it should be documented. The closest thing to an immutable list in Python is a tuple. Unless you see a compelling case for allowing frozenset or custom immutable types, I think we should just replace all references to "immutable lists" with tuple.

“Tuple” is fine. I don’t remember why I didn’t just use that. Background: the returned lists/tuples are immutable because we were running into situations like this:

font.groups[“group1”].append(“aGlyph”)

In RoboFab, manipulating the lists directly like this introduces a lot of changed state problems in the lower level objects. The Python list can’t be observed using the same techniques we use for our own objects. These observations are used to determine which parts of the data have changed, so direct changes to the lists were not flipping change states. When I wrote FontParts I wanted to avoid this and I had two choices: wherever list is used, switch the type to an internal ObservableList or make the lists immutable. Catching all of the lists was going to add a massive amount of code so I decided to just make everything tuple before it is returned. It was, ummmm, a controversial decision but I think I got it right.

“Tuple” is fine. I don’t remember why I didn’t just use that. Background: the returned lists/tuples are immutable because we were running into situations like this:
font.groups[“group1”].append(“aGlyph”)
In RoboFab, manipulating the lists directly like this introduces a lot of changed state problems in the lower level objects. The Python list can’t be observed using the same techniques we use for our own objects. These observations are used to determine which parts of the data have changed, so direct changes to the lists were not flipping change states. When I wrote FontParts I wanted to avoid this and I had two choices: wherever list is used, switch the type to an internal ObservableList or make the lists immutable. Catching all of the lists was going to add a massive amount of code so I decided to just make everything tuple before it is returned. It was, ummmm, a controversial decision but I think I got it right.

Thanks for the info. I'm no expert, but I think you got it right too. :)

@knutnergaard this looks great! I agree with Tal's notes. As for max line length, lets go with 80. Like Tal, I don't love this, but am ok with it. Agree with what you want to do for __init__. Let me know if you need help or have questions!

(FWIW black uses 88 chars for line length, which in practice is a pretty good value.)

@knutnergaard this looks great! I agree with Tal's notes. As for max line length, lets go with 80. Like Tal, I don't love this, but am ok with it. Agree with what you want to do for __init__. Let me know if you need help or have questions!

The main rationale for the short line length recommendations in the PEP is to better facilitate side-by-side diffing of files, which I personally think is a good reason to keep them.

There is also something to be said for the lower limit for docstrings compared to code, to keep the code as a more prominent sorrounding "frame" for the docstrings.

That said, 80 (or more) is fine with me. At least the font module adheres to the standard 79/72 characters limit pretty closely, though, so I will treat this as an allowed upper limit rather than an ideal line length, if you don't mind, just to minimize the need for reformatting.

(FWIW black uses 88 chars for line length, which in practice is a pretty good value.)

I haven't tried Black (or any other external code formatter for that matter). I assume you can adjust the line length and other default settings if needed?

Anyway, I'll check it out.

Black is very opinionated and offers only limited customization. General best practice (imo) is to use all default settings. I use it for every new project. So for me, 88 it is.

If we decide to run Black on fontParts, let's do that as a PR as it will be a bunch of changes unrelated to documentation. I really don't care but I guess it is good to have a standard, so if you want @typesupply I can do that and make it a post-commit hook or some such (what do you do @justvanrossum?)

If we decide to run Black on fontParts, let's do that as a PR as it will be a bunch of changes unrelated to documentation. I really don't care but I guess it is good to have a standard, so if you want @typesupply I can do that and make it a post-commit hook or some such (what do you do @justvanrossum?)

@benkiel @typesupply

Sounds good!

BTW, I'm done with the major documentation edits in font.py. I've defined all the type aliases needed for the annotations in a new module: fontParts/base/types.py. Unless you want me to define these elsewhere, I will submit what I have as a PR.

Also, there seems to be a few discrepancies between the contents of font.py and layer.py. Basically, dir(BaseFont) reveals a few public methods not defined in font.py, but imported from layer.py. Nevertheless, their private equivalents are defined in font.py. There are also a few attributes and methods which show up as either public or private BaseFont members in spite of not having any equivalent defined in font.py at all. This makes it kind of hard to get the documentation right or to know which methods or attributes are actually suppose to be referenced in environments/objects/font.rst, objectref/objects/font.rst or somewhere else entirely.

PR away, fontParts/base/type.py feels right. Could you list the discrepancies and we can sort it out

PR away, fontParts/base/type.py feels right. Could you list the discrepancies and we can sort it out

Actually, these discrepancies were a bit harder to trace exactly than I anticipated, but at least the methods _getGuideline and _getGuidelineIndex do not seem to have any equivalent public members in BaseFont or any of it's base classes.

Also, there seems to be a few discrepancies between the contents of font.py and layer.py. Basically, dir(BaseFont) reveals a few public methods not defined in font.py, but imported from layer.py.

This is a legacy support from before fonts had layers. glyph = font[name] is how we would get glyphs because there was only one layer. Now we can have more than one, so the layers API was introduced. To keep existing scripts from breaking I have glyph inquiry calls at the font level route to the default layer. This is also very useful outside of legacy support because, at least for me, > 90% of the time we're working on glyphs, they are the glyphs in the default layer. It’s a lot faster to type glyph = font[name] than glyph = font.defaultLayer[name].

_getGuideline

This is a method for subclasses to use to provide a guideline located at a specific index. It doesn’t have a direct public equivalent. It is used by the guidelines public property.

_getGuidelineIndex

This is a private method used by removeGuideline. It shouldn’t be in the public documentation.

Also, there seems to be a few discrepancies between the contents of font.py and layer.py. Basically, dir(BaseFont) reveals a few public methods not defined in font.py, but imported from layer.py.

This is a legacy support from before fonts had layers. glyph = font[name] is how we would get glyphs because there was only one layer. Now we can have more than one, so the layers API was introduced. To keep existing scripts from breaking I have glyph inquiry calls at the font level route to the default layer. This is also very useful outside of legacy support because, at least for me, > 90% of the time we're working on glyphs, they are the glyphs in the default layer. It’s a lot faster to type glyph = font[name] than glyph = font.defaultLayer[name].

Thanks for clarifying. I gather that e.g., the BaseFont.keys method, which is shared with the BaseLayer class, should be documented as being tied to the default layer.

_getGuideline

This is a method for subclasses to use to provide a guideline located at a specific index. It doesn’t have a direct public equivalent. It is used by the guidelines public property.

_getGuidelineIndex

This is a private method used by removeGuideline. It shouldn’t be in the public documentation.

Again, thanks for clarifying.

Thanks for clarifying. I gather that the BaseFont.keys method, which is shared with the BaseLayer class, should be documented as being tied to the default layer.

Yep.

Some of the methods in font.py and layer.py have the same arguments, but with different settings between public and private equivalents, e.g.:

def insertLayer(self,  layer: BaseLayer, name: Optional[str] = None) -> BaseLayer:
    pass

def _insertLayer(self, layer: LayerType, name: str, **kwargs: Any) -> LayerType:
    pass

Given that the default value of the public method provides the basis for specific, practical logic, is this an oversight, or is it intentional?

Given that the default value of the public method provides the basis for specific, practical logic, is this an oversight, or is it intentional?

Do you mean the **kwargs? That is intentional. It is in place to make it possible for us to add arguments to methods without immediately breaking existing implementations. For example, say we decide to add a color argument to the insertLayer method. The public method would become:

def insertLayer(layer, name=None, color=(1, 0, 0, 0.5)):
    # do what needs to be done to the data to normalize and prep for the subclasses
    self._insertLayer(layer=layer, name=name, color=color)

If the private method didn’t have the **kwargs, any implementation of _insertLayer that isn’t updated to include color will raise an exception because it will have an unknown argument. The **kwargs trick will allow the unknown argument to be silently absorbed. This violates the Zen of Python "Errors should never pass silently.” rule because the scripter isn’t warned that their data is being ignored. However, I was very concerned about the API becoming frozen because we’d have to do updates to all subclasses when we updated the public API. I invoked the "Unless explicitly silenced.” exception and moved on. In a more recent module I wrote I catch these situations via the inspect module and issue warnings. I don’t know that it is worth the effort to implement it in fontParts and I don’t know how much overhead that would add to script execution. Any overhead would be bad.

No, sorry for not bing clear. The kwargs is very logical. I'm alluding to name being optional in the public method, but not in the private one.

On the one hand, why would it be, unless there were predefined logic in the private method? On the other hand specifying name with a default of None may clarify the indended basic functionality.

No, sorry for not bing clear. The kwargs is very logical. I'm alluding to name being optional in the public method, but not in the private one.

Ah. This is also intentional. There shouldn’t be any optional arguments in the environment implementation methods. I can’t swear that there aren’t some now, but my intention at the start was that everything arriving to the environment methods would be defined and of the correct type. This way, any type conversion and defaults are handled at the generic fontParts level and this behavior will thus be consistent across implementations. It also alleviates a ton of work in the subclasses because all the type testing and conversion is a lot of redundant code. I was looking to see if I had written any notes about this and I found this in the documentation that might be helpful.

I also found a style guide I wrote for the documentation. It would be great if you could update this with your standard formatting.

Thanks again for doing this! I haven’t thought about most of this stuff since I wrote it however many years ago. 😄

No, sorry for not bing clear. The kwargs is very logical. I'm alluding to name being optional in the public method, but not in the private one.

Ah. This is also intentional. There shouldn’t be any optional arguments in the environment implementation methods. I can’t swear that there aren’t some now, but my intention at the start was that everything arriving to the environment methods would be defined and of the correct type. This way, any type conversion and defaults are handled at the generic fontParts level and this behavior will thus be consistent across implementations. It also alleviates a ton of work in the subclasses because all the type testing and conversion is a lot of redundant code. I was looking to see if I had written any notes about this and I found this in the documentation that might be helpful.

I also found a style guide I wrote for the documentation. It would be great if you could update this with your standard formatting.

Thanks again for doing this! I haven’t thought about most of this stuff since I wrote it however many years ago. 😄

Thanks! I'll have a look at those documents.

Being just about done with layer.py now as well, I have a few more questions, which I don't think they address:

Both BaseFont and BaseLayer include the methods getReverseComponentMapping and getCharacterMapping, as well as their private equivalents. None of the methods include any logic, however, and fontparts.world takes it's logic for these methods from Defcon. This logic does not seem to be in line with the 'immutable list' principle of FontParts, returning a Dict[str, Set[str]] in one case and Dict[int, List[str]] in the other. I also question why this logic isn't included in FontParts first hand. It seems logical to me that it should, if for no other reason than being instructive. I can copy the logic from Defcon, converting sets and lists to tuples if needed, before I submit the pull request of layers.py if desired.

I'm having an issue with displaying the type annotation of dynamic properties in the documentation correctly. Here's an example:

selectedLayers: Tuple[BaseLayer, ...] = dynamicProperty(
        "base_selectedLayers",
        """Get or set the selected glyph layers in the default font layer.

        :param value: The :class:`list` of :class:`BaseLayer` instances
            to select.
        :return: A :class:`tuple` of currently selected :class:`BaseLayer`
            instances.

        Example::

            # Getting selected layer objects:
            >>> for layer in layer.selectedLayers:
            ...     layer.color = (1, 0, 0, 0.5)

            # Setting selected layer objects:
            >>> layer.selectedLayers = someLayers

        """
    )

    def _get_base_selectedLayers(self) -> Tuple[BaseLayer, ...]:
        selected = tuple([normalizers.normalizeLayer(layer) for
                          layer in self._get_selectedLayers()])
        return selected

def _set_base_selectedLayers(self, value: List[BaseLayer]) -> None:
        normalized = [normalizers.normalizeLayer(layer) for layer in value]
        self._set_selectedLayers(normalized)

As you can see, the input and output types are correctly annotated for the getter and setter methods, but since the theme only gets the annotation for these properties from the actual variable, these types are not displayed. Annotating the variable with the return type, like I've done so far is technically not correct and may cause problems with type annotation in subclasses if the input and out types differ, like in the example. There may exist Sphinx plug-ins which may convert type declarations in the docstrings to signature-based type annotations, but I'm not sure. This conversion is commonly done at the theme level, but I don't know if this is the case for the FontParts theme, or what settings I would have to tweak to get there.

Seeing as the theme in this case is custom built, the best thing might be to fetch the annotations of dynamic properties from the getter and setter methods, displaying the input/output types in the same fashion as for regular methods and functions. Is that something can be built into the theme without too much effort?

Also, regarding the object-specific classes derived from BaseCompatibilityReporter, are these intended for subclassing or direct use by an object subclass, or is the explanation of the reporter parameter in the _isCompatible method as "An object used to report compatibility issues" an indication that such object is supposed to be entirely environment-based? Apart from general curiosity, I'm asking because this will have consequences on how the parameter type should be annotated.

Both BaseFont and BaseLayer include the methods getReverseComponentMapping and getCharacterMapping, as well as their private equivalents. None of the methods include any logic, however, and fontparts.world takes it's logic for these methods from Defcon. This logic does not seem to be in line with the 'immutable list' principle of FontParts, returning a Dict[str, Set[str]] in one case and Dict[int, List[str]] in the other. I also question why this logic isn't included in FontParts first hand. It seems logical to me that it should, if for no other reason than being instructive. I can copy the logic from Defcon, converting sets and lists to tuples if needed, before I submit the pull request of layers.py if desired.

You can copy the logic. That’s a good idea. You are right: the immutability probably should be handled consistently. It hasn't mattered to date because there is no setting equivalent for these. They are just helper methods.

Is that something can be built into the theme without too much effort?

I don’t know anything about the theme stuff, unfortunately. @benkiel Do you have any ideas?

Also, regarding the object-specific classes derived from BaseCompatibilityReporter, are these intended for subclassing or direct use by an object subclass, or is the explanation of the reporter parameter in the _isCompatible method as "An object used to report compatibility issues" an indication that such object is supposed to be entirely environment-based?

Ben, do you have any thoughts on this?

Both BaseFont and BaseLayer include the methods getReverseComponentMapping and getCharacterMapping, as well as their private equivalents. None of the methods include any logic, however, and fontparts.world takes it's logic for these methods from Defcon. This logic does not seem to be in line with the 'immutable list' principle of FontParts, returning a Dict[str, Set[str]] in one case and Dict[int, List[str]] in the other. I also question why this logic isn't included in FontParts first hand. It seems logical to me that it should, if for no other reason than being instructive. I can copy the logic from Defcon, converting sets and lists to tuples if needed, before I submit the pull request of layers.py if desired.

You can copy the logic. That’s a good idea. You are right: the immutability probably should be handled consistently. It hasn't mattered to date because there is no setting equivalent for these. They are just helper methods.

Done! I eliminated all the extra defcon-specific stuff and simplified the logic further by using collections.defaultdict, which is also slightly more efficient than initializing each default value separately. I also took the liberty of updating RLayer similarly.

This raises another question, though, of whether the selectedLayers property (and other similar cases) really should demand a tuple instead of a list. I can't really see any reason why not, except that it might be convenient for certain environments.

would say that selectedLayers should be able to take in a list, but change that list to a tuple

Is that something can be built into the theme without too much effort?

I don’t know anything about the theme stuff, unfortunately. @benkiel Do you have any ideas?

Will have to look into it — @arrowtype did most of the theme work, but that was years ago. If the custom theme is going to be a huge hassle, I am ok changing the theme to something that might be easier. Give me a bit to dig into theme stuff.

Also, regarding the object-specific classes derived from BaseCompatibilityReporter, are these intended for subclassing or direct use by an object subclass, or is the explanation of the reporter parameter in the _isCompatible method as "An object used to report compatibility issues" an indication that such object is supposed to be entirely environment-based?

Ben, do you have any thoughts on this?

The way we have it set up is that _isCompatible right now does all of the work, but if an environment wants to override that code it can. You'll see this in other places too: where we can provide a generic environment version of a private method we do, to make implementing fontParts less work.

So, I guess the answer to the question is that the object-specific classes are intended to be directly used by an object subclass.

@benkiel sorry, this is a long thread and I'm on the go, so it's hard to catch up on the whole thing. What are we trying to adjust in the theming?

@arrowtype sorry, indeed it's a huge read. Issue is here: https://github.com/robotools/fontParts/issues/207#issuecomment-2282921183

Question is:

Seeing as the theme in this case is custom built, the best thing might be to fetch the annotations of dynamic properties from the getter and setter methods, displaying the input/output types in the same fashion as for regular methods and functions. Is that something can be built into the theme without too much effort?

I know that you built this at this point almost 10 years ago, so no worries if nothing comes to mind.

Ohh hmm, yeah, I think that even when seeing up the theme, I mostly just worked with CSS, and didn't adjust the structure or content by much (though I can't remember it super clearly). I can possibly take a closer look on Friday, though I'm also happy if it's time for the theme to move onto something more practical for the current state of the project.

Okay, having read through more of the thread now... I realize it’s out of my current depth to implement the proposed change in the theme. If someone else implements that logic, I’d love to see what the solution is, and I’d be happy to help with the visual styling of it if it’s done in the current theme, but I don’t know where I’d start in trying to extend the theming logic like that. Sorry I can’t be more helpful here!

robotools / fontParts

Check Object documentation for Font #207