Fontbakery: Improve metadata specification introducded in PR #1583

[graphicore]

In #1583 you (@felipesanches ) introduced new metadata arguments to fontbakery.callable.FontBakeryCheck. We need better specifications of the expected data types and use cases. Inline comments are in general fine, BUT the other arguments are already described in the docstring of the Class.

Please move the specs and flesh out:

• [ ] Move the description comments into the docstring of FontBakeryCheck.
• [ ] rationale what is the difference to documentation how should each be used (this would be sth. I'd just put into documentation)
• [ ] affects please add an example, maybe define commonly needed words, so that we don't end up with Windows, windows and win meaning the same thing or describe how to derive a key here.
• [ ] affects specify the format of the affected versions range or mark it explicitly as a TODO.
• [ ] affects Make it a tuple of tuples, we don't want to change it, a tuple makes this semantically more clear.
• [ ] affects In the first test that uses the spec: ('Windows', 'unspecified') The second value should be None if unknown (see above: specify affected versions range.)
• [ ] example_failures how is this formatted and expected to be used, I can't deduce from the current description. Why can't this be in rationale or documentation? Maybe mark it explicitly as a TODO.
• [ ] In the first test that uses the spec, remove the line , example_failures = None. It's optional and shouldn't be used explicitly if it has no value.

[davelab6]

It would be nice to structure the next release late this year around this being fully deployed across the codebase

[felipesanches]

The second value should be None if unknown (see above: specify affected versions range.)

It seems confusing and likely misleading to read code like affected: None while the actual intended meaning is affected: "unspecified" or, in other words, that the affected systems and versions are all still not explicitly determined.

[felipesanches]

I'd suggest (for code legibility) declaring something like:

# Lib/fontbakery/constants.py
# This constant denotes that the specific version range of systems affected by an issue is still not well determined.
UNSPECIFIED = -1

# Lib/fontbakery/specification/googlefonts.py
from fontbakery.contants import UNSPECIFIED
...
affected = UNSPECIFIED
...

[felipesanches]

In the first test that uses the spec, remove the line , example_failures = None. It's optional and shouldn't be used explicitly if it has no value.

The explicit use of None here was my means of suggesting its usage. The idea is that as other checks start using this, this first example usage (with None) can be removed. I'll keep it for now.

[graphicore]

None is semantically perfectly fine to say "unspecified", I don't understand how your constant makes it any better.

[felipesanches]

One important note, though. I prefer to evolve this kind of thing organically. I wouldn't put much effort into coming up with a formal description of these metadata fields. Instead, I'd gradually adopt some, and drop others as I see fit. The wider the adoption of metadata fields, the better coherent they'll get to be (with time).

I will remove the release-specific milestone from this issue, because this aspect of the software is meant to evolve at a slower pace.

[graphicore]

he explicit use of None here was my means of suggesting its usage.

But you are overriding the default, e.g. you can't change the default anymore. Not too bad though.

[felipesanches]

None is semantically perfectly fine to say "unspecified", I don't understand how your constant makes it any better.

It reads like quite the contrary. Regardless of python code conventions (which is what I assume is your point of view), in an english sentence affected: None reads like: "none of the systems/versions are affected".

[graphicore]

I'd gradually adopt some, and drop others as I see fit.

Please only add fields that have an actually usage when added and can be speccred. We don't need speculative fields for future use that are underspecified.
Feel free to suggest such unused proposals in the doc-string.

Adding unused elements to a public API is a maintenance nightmare!

[graphicore]

The point is, that changing public APIs is MUCH more complicated and error-prone than private ones. In this cases, changes will screw everyone who implements an own spec. We won't be able search and replace, because we won't know who uses it and where the code is. Please don't add stuff that has no apparent use or is underspecified.

[felipesanches]

I don't see the harm you try to hint at here since no one actually uses something that is of vague purpose, so there's no legacy code to maintain. There's no maintenance nightmare if there's no active external usage.

The bulk of these metadata fields can be thought of as simply free-form metadata tags.

[felipesanches]

But I think there's a solution to your worries! We can add a single metadata field which is explicitely specified as a dictionary of:

Free-form metadata values. If recurring usage patterns emerge, these may be promoted into first-class fields for specific purposes after being formalized into the spec.

[graphicore]

… since no one actually uses something that is of vague purpose, so there's no legacy code to maintain. There's no maintenance nightmare if there's no active external usage.

But our aim is to get others to use this and fucked up APIs make this much harder.

The bulk of these metadata fields can be thought of as simply free-form metadata tags.

Then make a metadata free form argument and put that shit there and don't pollute the API. I will need these interfaces for tooling. If you don't have any use for them, don't define them. We can always later add a well defined interface.

[graphicore]

LOL

[felipesanches]

we could call it tagsperhaps, but I think semantically that's a bit restrictive. It seems that calling is metadata denotes a more general purpose. It could even be called misc_metadata if you like.

[felipesanches]

it seems we're suggesting the same thing :-D

[graphicore]

Maybe custom?

[graphicore]

I don't like tags so much it feels like we could use that for something in the future (i.e. a selection mechanism). misc_metadata, metadata, custom_data, custom etc. s all good for me.

[graphicore]

Regardless of python code conventions (which is what I assume is your point of view),

it is

in an english sentence affected: None reads like: "none of the systems/versions are affected".

Why would you mention it at all if "none of the systems/versions are affected"? My point here is also, that as a version a type of Number or String may be thinkable/needed and None it is neither.

Also, there will be documentation that can simply state: "If you don't know the affected version number use None". This is input into a computer program, for an interface targeted at none-programmers, we can easily format None as unspecified or whatever.

[graphicore]

We just discussed that using a front-matter yaml as in jekyll in the docstring could nicely solve the issue. I'll follow up on this.

[felipesanches]

I looked up this "front-matter yaml" thing this afternoon but we already have a good solution for our needs. All the data is already available on runtime and now better organized with the misc_metadata field suggested by Lasse and adopted on the latest commit above.

We should focus efforts on the real important thing which is collecting more metadata such as rationales for the checks rather than the technology adopted or syntactic sugar.