Improve yaml models: ConfigTasks #122
Conversation
| if not isinstance(config, models.ConfigIconTask): | ||
| raise TypeError |
There was a problem hiding this comment.
This check is here to satisfy mypy, because we can not put this as a type hint in the signature (would break Liskof substitution principle -> mypy complains). Not necessary for ShellTask because we really check in Task that the subclasses get the correct types.
src/sirocco/core/_tasks/icon_task.py
Outdated
| @@ -93,3 +97,17 @@ def section_index(section_name) -> tuple[str, int | None]: | |||
| if m := multi_section_pattern.match(section_name): | |||
| return m.group(1), int(m.group(2)) - 1 | |||
| return section_name, None | |||
|
|
|||
| @classmethod | |||
| def build_from_config(cls: type[Self], config: ConfigTask, **kwargs: Any) -> Self: | |||
There was a problem hiding this comment.
Also, just for my understanding: We're supporting Python >= 3.10, but Self type was introduced in 3.11. Is that an issue, or can we use newer Python features in type hinting? According to ChatGPT:
"""
You're correct—Self was introduced in Python 3.11, not 3.12. Since your package claims to support Python 3.10+, using Self directly would be a problem for users on 3.10.
Can you use newer Python concepts in type annotations?
Yes! Starting with PEP 563 (Python 3.7) and PEP 649 (Python 3.10), Python supports postponed evaluation of annotations, meaning type hints are stored as strings instead of being evaluated immediately. This allows you to use newer type hints while keeping compatibility with older versions, as long as they aren't used at runtime.
Solution:
If you add this import at the top of your module:
from __future__ import annotations
Then the Self annotation will be treated as a string ("Self") and won't cause syntax errors in Python 3.10. However, tools like mypy will still understand it.
"""
There was a problem hiding this comment.
GPT is correct in that it was introduced later and in that in our case it works (at runtime) because it is treated as a string. Here's how that applies to Sirocco:
For now, we have set the hatch project to run type analysis on 3.12 and we use the future annotations. Therefore it is type checked as if it was supporting 3.12+ (ok at analysis time), while the interpreter treats all the type hints as just strings (ok at run time).
If we were to lower that to 3.10, we would have to import Self from typing_extensions (making that a dependency), if we choose to keep using it. It would also affect other things (not touched in this PR) like class Store[T], which would have to turn into class Store(typing.Generic[T]). The backports in typing_extensions can catch some of this but not syntax changes like class Store[T] (which I am honestly suprised that it works at runtime). I am also wondering why from typing import Self does not cause an import error in 3.10, but it seems not to.
| return data | ||
| if len(data) == 1: | ||
| key, value = next(iter(data.items())) | ||
| match key: |
There was a problem hiding this comment.
Interesting. Haven't really used such match/case constructs before, but makes the structure clearer than using isinstance.
There was a problem hiding this comment.
I agree. Now, if there was a match expression, the result of which can be assigned to a variable (like in Rust), I would love it even more...
Co-authored-by: Julian Geiger <[email protected]>
Fix #84:
ConfigNamelist_NamedBaseModelkey extraction class method to a free function for reuse inConfigNamelistConfigIconTaskvalidator_tasks.Tasksubclasses to hook into how they are created from the equivalentconfig.ConfigTasksubclass.config.ConfigIconTask->_task.IconTask.General Improvements:
_yaml_data_models.py->yaml_data_models.pyto reduce conflicts between merges of Refactor dates handling #121 and Improve yaml models: ConfigTasks #122