feat: improve internal typings in manifest#278
Open
IzaakGough wants to merge 3 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request improves type safety in manifest.py by introducing strict type definitions (ManifestParamBase, SpecValue, etc.) and refactoring helper functions to use them. Two critical issues were identified in the review: first, using a generic alias (list[ManifestParamBase]) as a default_factory in ManifestStack will raise a runtime TypeError; second, the new strict type checking in _object_to_spec will raise a TypeError for previously supported types like tuple and custom mappings, which should be explicitly handled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_typing.Any.Problem/Root Cause
Issue #31 called out that the manifest internals still relied on
_typing.Any, which left the manifest serialization path loosely typed even though it operates on a constrained set of values. The previous implementation useddict[str, _typing.Any], untyped helper inputs, and a narrowlistcheck in_object_to_spec, so type information was lost across manifest parameter conversion and dataclass serialization.Solution/Changes
This change introduces explicit aliases for supported manifest parameter types and manifest spec values, then threads those types through the manifest conversion helpers. The serialization path now distinguishes dataclass instances, mappings, and non-string sequences, which preserves support for inputs such as tuples while keeping the output normalized to manifest-compatible lists and dicts. It also updates the shared
_paramsregistry type so stored params are typed asParamorSecretParaminstead of a genericExpression.Testing
format,lint,docs,Analyze,CodeQL, andtest (3.10)/test (3.12).