Repository navigation
Native types cli integration - #866
Conversation
44b38e6 to
58549c6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Accepted import package names can produce invalid distribution metadata, leaving generated packages unbuildable.
1 open issue
What changed in this PR
Completes the native standalone types CLI pipeline, building on lazy model generation in #863 and advancing the Python generator migration in #797.
Changes:
- Connects types-specific CLI settings to selection, schema generation, and lazy model generation.
- Generates package metadata, initializers, and typing markers, then publishes through the file manifest.
- Adds CLI coverage and updates the versioned output snapshot.
| File | Description |
|---|---|
| scripts/codegen-output-py/generate.py | Generates the snapshot through the CLI. |
| packages/smithy-python/tests/unit/test_selection.py | Updates selection tests for working types generation. |
| packages/smithy-python/tests/unit/test_cli.py | Tests package output, settings, and validation. |
| packages/smithy-python/src/smithy_python/types.py | Assembles complete standalone types packages. |
| packages/smithy-python/src/smithy_python/cli.py | Adds types options and invokes generation. |
| packages/smithy-python/README.md | Documents available types generation. |
| codegen-output-py/types/src/example_types/py.typed | Marks the generated package as typed. |
| codegen-output-py/types/src/example_types/_private/__init__.py | Initializes the private package. |
| codegen-output-py/types/src/example_types/__init__.py | Exposes the generated package version. |
| codegen-output-py/types/README.md | Adds generated package documentation. |
| codegen-output-py/types/pyproject.toml | Adds build metadata and tooling settings. |
| codegen-output-py/types/.smithy-python-manifest.json | Tracks the additional generated files. |
Review effort: Balanced
58549c6 to
99e8ee5
Compare
99e8ee5 to
91f5e28
Compare
df06c4c to
c93800f
Compare
c93800f to
3a125c1
Compare
3a125c1 to
73d6de9
Compare
b3bef1e to
5220488
Compare
5220488 to
230257e
Compare
230257e to
cb5f272
Compare
a02e67f to
b3b28a8
Compare
* add lazy model generation * test streaming blob model generation * support event stream unions in model generation * exclude eventstream members from generated structs * filter event stream members from generated stubs * filter event streams from generated typecheck fixtures
b3b28a8 to
3dfcbe7
Compare
| @@ -0,0 +1 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
I think we only need this in one or two files. I don't think it should be getting added unconditionally. Maybe we should just be responsible for adding it where we need it for now? Then add some smart detection in the future?
There was a problem hiding this comment.
yeah, this is coming from PythonWriter. I’ll make it opt-in and only add it to files that need deferred annotations
There was a problem hiding this comment.
I don't think we need it anymore now that we load models lazily. Can we remove the future_annotations option all together (and references to it from designs)? I'd rather add it later if we ever do need it as opposed to carrying this option around we never use. Or it's just something we should write as generated code the way we should another import
| reportPrivateUsage = false | ||
| reportUnusedFunction = false | ||
| reportUnusedVariable = false | ||
| reportUnnecessaryComparison = false | ||
| reportUnusedClass = false | ||
| enableExperimentalFeatures = true | ||
| exclude = ["**/models.py"] |
There was a problem hiding this comment.
[not blocking] I know these are probably inherited from the java generator, but do you know why they're needed here? A few of these like AreportUnusedFunction and reportUnnecessaryComparison seem weird. I also don't get exclude = ["**/models.py"].
There was a problem hiding this comment.
I checked these individually and they aren’t needed. the models.py exclusion is hiding a computed __all__ warning, so I’ll fix that and remove the inherited settings
There was a problem hiding this comment.
We might need enableExperimentalFeatures = true later. So we can either keep that one now or add it later when it comes up. I don't think it's something we can forget to do, its something we'll need to do so later seems fine
| ] | ||
| dependencies = [] | ||
| dependencies = [ | ||
| "packaging", |
There was a problem hiding this comment.
I don't feel good about taking a dependency on packaging just for an error and version validation (e.g. from packaging.version import InvalidVersion, Version). Is this something we can do with a regex or something?
There was a problem hiding this comment.
agreed. I’ll validate the version format with a conservative regex and drop the dependency instead of supporting every possible PEP 440 form
| help="Smithy selector used to narrow generated declaration roots.", | ||
| ) | ||
| types.add_argument( | ||
| "--generate-inputs-and-outputs", |
There was a problem hiding this comment.
nit - Thoughts on using something like --include-operation-io instead?
There was a problem hiding this comment.
--include-operation-io reads better to me. I’ll rename it
b13ff90 to
7a227d6
Compare
jonathan343
left a comment
There was a problem hiding this comment.
Approving. I'd like to see future_annotations removed, but not blocking.
7a227d6 to
4f01769
Compare
4f01769 to
573ffa8
Compare
Issue #, if available:
#797
Description
Completes the native standalone types pipeline by wiring schema and lazy model generation into
smithy-python generate types.codegen-output-pysnapshot.This PR is stacked on #863.
Follow-up work
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.