Add type annotations to core classes and plugin infrastructure#3893
Add type annotations to core classes and plugin infrastructure#3893arunpersaud wants to merge 7 commits into
Conversation
felixfontein
left a comment
There was a problem hiding this comment.
Thanks for working on this!
I guess this shows that adding type hints to an existing non-trivial Python codebase is hard. (I've been struggling with that myself more than once, even if you don't have the trouble of attributes that aren't always there.)
Regarding the attributes that aren't always there: maybe we should change them to attribute: type | None and initialize them with None?
Also regarding attribute type hints: I (nowadays) think it's better to add their types in __init__ than already in the class definition. The rationale is that when they are defined on the class level, the type checker won't notice if you use them too early in __init__ (before actually setting them). (They might still notice, I don't know how sophisticated they are nowadays, but better safe than sorry.) Also it's harder to forget to actually set an attribute that was declared to exist. Especially for complex constructors like Nikola.__init__ with its many hundreds of lines, this can make a difference.
There was a problem hiding this comment.
Since the type is above, it should be removed here:
| self._template_system = None |
| plugin_manager: PluginManager | ||
| _template_system: TemplateSystem | ||
| _template_system: TemplateSystem | None | ||
| tzinfo: datetime.tzinfo | None |
There was a problem hiding this comment.
How can tzinfo be None?
There was a problem hiding this comment.
tzinfo is set by dateutil.tz.gettz, which returns None on a non-valid timezone request
>>> dateutil.tz.gettz('asdfsadf') is None
True
There was a problem hiding this comment.
This is using
>>> dateutil.__version__
'2.9.0.post0'
Perhaps in addition to the try-except block NIkola should check for None and do the same as in the try except?
|
As for moving things to init. Happy to look into this. The main Nikola class now has And I think some, for example, I could add this here or perhaps as another PR in the future for more work on the typing. Let me know what you prefer |
|
The current lint failure is from one unused import line in The class annotation in this PR uses import datetime
-from datetime import timezone, tzinfo
import ioThe rest of the CI matrix is green. |
Pull Request Checklist
Description
Adds type hints for site, logger, and various Nikola class attributes to improve type checking. Testing using pytest and 'ty'