Optional arguments in environs implementation methods

[knutnergaard]

Ref. this post:

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.

Originally posted by @typesupply in #207 (comment)

There are indeed a few cases of default values in the environment implementation of methods. If desired, I can convert them to be absolute as I come across them while implementing the type annotation and working on the docs.

Also, when running mypy, there will be a conflict between the public and private methods whenever one employs optional values and the other does not. The best thing, unless you want to ignore this, is to implement a conditional to eliminate the possibility of None wherever it's incompatible. I can do this too if you wish.

[benkiel]

Yes, please do both of these things!

[knutnergaard]

@benkiel, the BaseGlyph class has a few cases where default None values in the environment implementation method are actually documented in the docstring (e.g., in the _appendGuideline method). Can you confirm that these also should be absolute?

[benkiel]

Hey @knutnergaard I'm unsure what you mean by absolute in this context, could you elaborate?

[knutnergaard]

Hey @knutnergaard I'm unsure what you mean by absolute in this context, could you elaborate?

@benkiel, sorry, absolute is probably not the right word. I simply mean mandatory or non-optional.

[benkiel]

Yes, we should note the additional ones in the private method.

[knutnergaard]

Yes, we should note the additional ones in the private method.

I'm not sure I understand, but I take it to mean that documented optional values, None or otherwise, are intentional, contrary to the general idea laied out by @typesupply in the OP, and should be kept as is?

[benkiel]

Sorry, no; we should move those optional things up to the non-private method and document there. Sorry, I lost the thread on this; please do what Tal said and ignore me!

[knutnergaard]

@benkiel, @typesupply, sorry for harping on this further, but it just occurred to me that we might be talking past each other a bit here.

By «optional» arguments, are we talking about predefined keyword arguments or optional types in the way typing.Optional regards them, as either any specified type or None?

[benkiel]

@knutnergaard AH! Yes, I was talking about optional as predefined keyword args: ie, identifier=None is in _appendGuideline but not in appendGuideline, with is as intended from looking through the code (correct me if wrong @typesupply). I see you are referring to the **kwargs, which is unique to _appendGuideline and should be generically typed so environments can do whatever they need to there.

[knutnergaard]

@benkiel Thanks, although when you say:

identifier=None is in _appendGuideline but not in appendGuideline

I assume you mean the other way around, i.e., that the public methods should have default arguments, but not the private ones?

[benkiel]

Yes, you are correct, we should just have identifier in the private method, as the public method passes all values to the private one

[knutnergaard]

@benkiel Removing the default values for certain methods breaks the code in a few places, at least in glyph.py. Particularly, _toMathGlyph or _fromMathGlyph, which are called elsewhere in the module without specific arguments presents problems when their defaults are removed. Should I just specify appropriate arguments whenever they're called in the code instead?

[benkiel]

@knutnergaard Yes, we should do that, thank you!