chore: add type annotations to cloudinit.distros.parsers.hosts#6916
Conversation
Enable mypy check_untyped_defs for cloudinit/distros/parsers/hosts.py and its test module by adding full type annotations. Changes: - Replace None-sentinel _contents with a _parsed bool flag and an empty-list default, matching the pattern used in resolv_conf.py (canonicalGH-6909) - Annotate all method signatures with parameter and return types - Use List/Tuple/Any from typing for Python 3.8 compatibility - Rename shadowed variable in __str__ to avoid reassignment type mismatch - Change add_entry to store option components as a list instead of a tuple to keep _contents type consistent - Remove cloudinit.distros.parsers.hosts and tests.unittests.distros.test_hosts from the mypy check_untyped_defs=false overrides in pyproject.toml Closes part of canonical#5445
holmanb
left a comment
holmanb
left a comment
There was a problem hiding this comment.
This looks like it is going the right direction, thanks. I left a few nitpicks. Please make sure to sign the CLA. Also please run tox -e do_format to fix formatting issues.
| pieces = [str(p) for p in pieces] | ||
| pieces = "\t".join(pieces) | ||
| contents.write("%s%s\n" % (pieces, tail)) | ||
| (raw_pieces, tail) = components |
There was a problem hiding this comment.
Please remove the parenthesis around the assignment - they are not needed.
| contents.write("%s%s\n" % (pieces, tail)) | ||
| (raw_pieces, tail) = components | ||
| str_pieces = [str(p) for p in raw_pieces] | ||
| contents.write("%s%s\n" % ("\t".join(str_pieces), tail)) |
There was a problem hiding this comment.
I think an f-string here would make this easier to read.
| def _parse(self, contents: str) -> List[Tuple[str, List[Any]]]: | ||
| entries: List[Tuple[str, List[Any]]] = [] |
There was a problem hiding this comment.
Is there any reason this can't be List[Tuple[str, List[str]]]?
There was a problem hiding this comment.
Good question. The inner list can't be List[str] because option entries store [head.split(None), tail] - that's [list[str], str], so the two elements are different types. Changing it to List[str] would make mypy flag every place we do pieces, _tail = components (it would see a list of strings being unpacked into two str variables, losing the fact that pieces is itself a list). List[Any] is the minimal correct annotation here without introducing a custom TypeAlias or a NamedTuple. Happy to tighten it further if you'd prefer a TypeAlias approach.
| def parse(self): | ||
| if self._contents is None: | ||
| def parse(self) -> None: | ||
| if not self._parsed: |
There was a problem hiding this comment.
Rather than introducing the new attribute self._parsed, can we just check the truthiness of self._contents?
- Drop the _parsed flag; rely on truthiness of _contents instead (an empty list is falsy, so parse() only runs once on a non-empty file) - Remove unnecessary parentheses from all tuple-unpacking assignments - Swap the %-format in __str__ for an f-string (extract join to a variable first, since backslash in f-string expressions requires 3.12+) - Remove spurious parens around single-arg % format in blank/all_comment branches (black-clean, no behaviour change)
|
@PhinehasNarh It looks like a CLA signature is still required. |
Line 57 of test_cc_phone_home.py was 89 characters, causing ruff E501 in the check_format CI step. Trimmed the trailing clause from the comment; the context above (GH pytest-dev/pytest#14650 reference on the next line) already explains the full intent.
|
Thanks for the nudge. The CLA is still outstanding on my end - I'll get that signed. On the |
|
I think the main branch needs to be merged in. |
|
Done - just merged main in (resolved a conflict in |
mypy now checks test_hosts.py (removed from the override list in this PR). Line 24 reused 'eh' for str(eh), causing an incompatible-types error since mypy inferred 'eh' as HostsConf from the construction on line 18. Rename to 'eh_str' to give the string its own binding.
|
Pushed a fix for the mypy errors - |
2 participants
Summary
Adds full type annotations to
cloudinit/distros/parsers/hosts.pyso it can be removed from thecheck_untyped_defs = falsemypy override inpyproject.toml. Part of #5445.None-sentinel_contentswith a_parsed: boolflag and an empty-list default, consistent with the approach used inresolv_conf.py(chore: add typing to cloudinit.distros.parsers hostname and resolv_conf #6909), to avoid needingOptionaland narrowing assertsList,Tuple,Anyfromtypingfor Python 3.8 compatibility__str__wherepieceswas reassigned across incompatible types; use distinctraw_pieces/str_piecesnames insteadadd_entryto store option entries as a list rather than a tuple, keeping_contentstype consistent with what_parseproducescloudinit.distros.parsers.hostsandtests.unittests.distros.test_hostsfrom the mypy override blockTest plan
tests/unittests/distros/test_hosts.pypass unchangedmypy cloudinit/distros/parsers/hosts.pyreports no errors