Do I need to write a test for this? The changes feel pretty trivial.

Do I need to write a test for this? The changes feel pretty trivial.

Please do, it's worth checking that this code can be triggered.

Do I need to write a test for this? The changes feel pretty trivial.

Please do, it's worth checking that this code can be triggered.

I just took a look at a test for this. The test_fallback_missing_glyph_warn_family_names test covers the same ft_glyph_warn code path for the common case (real family names) and would catch any regression that broke the formatting logic entirely. Directly testing the null branch would require either exposing FT_Select_Size to Python or adding a dedicated C-level test hook, both of which feel excessive for the 1 line change.

@story645 Any thoughts? If the testing requirement is stringent and you can think of any alternatives/somebody else wants to take it over, I can also close it out.

or adding a dedicated C-level test hook, both of which feel excessive for the 1 line change.

does that mean the error message "unknown error" never gets propagated up?

And I think here the testing requirement isn't that stringent - looks like the coverage decreases are unrelated.

does that mean the error message "unknown error" never gets propagated up?

After digging a little bit, the only font format where FreeType would reutnr a null family_name is BDF (bitmap fonts without a FAMILY_NAME property), but matplotlib's set_size calls FT_Set_Char_Size which fails for bitmap fonts, so you can't size the face and trigger a glyph warning through the normal API.

The null check is really more just preventative but seems like it may honestly just never be triggered. From a correctness standpoint, it won't hurt.

I am a bit unconvinced by the need to remove NULL in at least the error list case.

The docs for the correlating Freetype function specifically say it can return NULL:

https://freetype.org/freetype2/docs/reference/ft2-error_enumerations.html

The example code for how to interact with these also includes a NULL (though interestingly only in one of the two examples, and not in the version that is most similar to what we have with a switch statement (which does not have a default case at all)

Given that NULL is an expected, documented return value here, I do not see the need to preemptively remove a NULL.

I am a bit unconvinced by the need to remove NULL in at least the error list case.

The docs for the correlating Freetype function specifically say it can return NULL:

https://freetype.org/freetype2/docs/reference/ft2-error_enumerations.html

The example code for how to interact with these also includes a NULL (though interestingly only in one of the two examples, and not in the version that is most similar to what we have with a switch statement (which does not have a default case at all)

Given that NULL is an expected, documented return value here, I do not see the need to preemptively remove a NULL.

Sorry if I'm not following but given the fact that we expect a NULL shouldn't we guard against that in order to avoid de referencing a NULL pointer?

I am referring to the change to FT_ERROR_END_LIST in my previous comment, which removes a return NULL in a usecase where NULL is an expected, documented return.

I am not referring to the check when dereferencing.

I am referring to the change to FT_ERROR_END_LIST in my previous comment, which removes a return NULL in a usecase where NULL is an expected, documented return.

I am not referring to the check when dereferencing.

Ah ok, I see what you're getting at. Leaning towards your argument; can revert the change if others agree but honestly this feels like a relatively uneventful, trivial change.

This is a weird one, because we aren't actually using FT_Error_String but a wrapper, and I'm not entirely sure why. It's possible that @anntzer did that because of this note in the docs:

FreeType has to be compiled with FT_CONFIG_OPTION_ERROR_STRINGS or FT_DEBUG_LEVEL_ERROR to get meaningful descriptions. ‘error_string’ will be NULL otherwise.

However, since we aren't using FT_Error_String, but our own internal ft_error_string, the docs about NULL return do not matter here.

FT_Error_String is relatively new (it's there since FT 2.10.0, which we still don't actually require). I guess we could bump the required FT version, though.
Returning "unknown error" in that case (which is only ever used by THROW_FT_ERROR) also makes sense to me.

FT_Error_String is relatively new (it's there since FT 2.10.0, which we still don't actually require). I guess we could bump the required FT version, though.

Yea, probably we should tighten up what version is required there, but that's a separate question.