Add config module as per #2349 #2416
Conversation
|
I tried to move the This could be solved by:
@anthrotype or @behdad would you be able to advise please? |
|
The first option -- "listing all options in the config module instead of in the various modules" -- is the easiest, though probably not the most elegant. What about, instead of needing to have calls to |
That sounds OK except it's not Python code, so one would not be able to implement parsing/validation logic (or one would have to express that declaratively, like with JSON Schema for example). For that reason I would say it would be better as Python code. Also, I was quite happy with the idea that you could import the config options into your code, to refer to them without having hardcoded strings in the code; that wouldn't be simple with the text-file-approach. Maybe the first option can be made to look organized enough by having |
can you make an example of what you have in mind? I thought the options values would be simple str, float, int, or stuff that you can store as json.
Not sure what you mean by hard coded string. You could still import the default config object and get the already-parsed values from it. |
yeah, if you actually need to have code to register options, then that could be an option to keep it organized. I'm still not 100 % that a declarative approach wouldn't be sufficient for our use case. |
Yes it's indeed very simple stuff, but still, assuming people can give options by writing them in a config file, we should expect to get the wrong type or values out of range. JSON schema would handle these cases for sure. See the tests below: parsing an input string to int, checking that the value is in the expected range. fonttools/Tests/config_test.py Lines 44 to 58 in bd0e156
See the two tests below: you have the fonttools/Tests/config_test.py Lines 20 to 30 in bd0e156 |
|
Collecting options from source into a toplevel config.py module is another option. We can make that run in CI and fail CI if config.py changed. |
|
I'll comment on the rest of the approach soon. Thanks! |
|
Cosimo: don't perform conversions from string to int in the default constructor; have a special one for par 8000 sing from file that does the conversions. In general you wouldn't expect an object that accepts an int value to also accept a string value and convert it. |
|
Instead of reading configuration from files, we could read it from a lib key in the designspace / UFO or a custom parameter in Glyphs.app source. E.g. in designspace: <lib>
<key>org.fontTools.config</key>
<dict>
<key>fontTools.otlLib.optimize.gpos.mode</key>
<int>8</int>
</dict>
</lib>in ufo2ft: ttFont = TTFont(config=designspace.lib.get('org.fontTools.config', {})) |
|
Cosimo: should this module be in misc? It's not font-related Also maybe there's an off-the-shelf thing in general for all this? |
It can come from there as well, yes.
I find |
|
I feel like the module is fundamental enough that should be at toplevel As for the TTFont property, at the risk of bikeshedding, I suggest we call it |
|
|
@anthrotype @behdad if you agree with how it is at the moment I'll start adding other options. I would also be happy to know which options you have in mind to add; I'm think to start with the parameters to TTFont(). |
|
Thanks Jany. This is concise and good. I have some ideas forward. Let me sketch those. |
|
Copy/pasting from chat to Jany/Cosimo: Jany, Cosimo, I have some Evil config ideass 😀 then accept a tuple: (path,node): in short, you can then do: and avoid repeating any classtype or module can be passed there. better yet, instead of a tuple, we can accept a colon as Cosimo liked 😛 Next. If only a path is passed and no node, we return the subconfig. That is, Finally, and the main way to use the config is to use to populate key/value arguments of constructors, methods, and functions. The syntax for that I have not fully figured out yet. That would pass all relevant cfg to method. Anyway; this is what's been brewing in my head. As for your Option type, I want use the typing module types directly. No need for extra Option type. Behdad Esfahbod, Jany Belluz, That sounds a bit complicated to be honest, but maybe it looks nice when using it in the code Behdad Esfahbod, Jany Belluz, (sounds like how a Ruby developer would think, not a Python one) Behdad Esfahbod, I'm not sure mine are good. Evil it might be. But want to explore 😀 Jany Belluz, |
|
@anthrotype I rebased on latest main |
|
@anthrotype I fixed all your comments and added the new module to the Sphinx documentation. I had an idea: how neat would it be if the list of registered options was parsed by Sphinx and added to the documentation page as well? I had a quick look and it sounds possible by adding some Sphinx code with new nodes and stuff, but I didn't go down the rabbit hole. Just a thought for later :) |
that would be nice, yeah. File an issue so we don't forget |
|
Done for the classvar and the issue |
|
@anthrotype I wrote release notes in the description |
See issue #2349
configToolsmodule, a generic configuration systemconfigmodule, a fontTools-specific configuration system using the above + tests + docConfigobject toTTFont