-
Notifications
You must be signed in to change notification settings - Fork 255
examples: fix mypy errors in example entry points; add local type-check script (#1091) #1248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanups!
| @cocoindex.op.function() | ||
| def markdown_to_html(text: str) -> str: | ||
| return _markdown_it.render(text) | ||
| return cast(str, _markdown_it.render(text)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about also considering code simplicity/readability. Given these examples are for users to understand how to use cocoindex, this matters.
For case like this, it's essentially the underlying library doesn't have a specific type, and the return type of the current function is clear.
I think we can just ignore it by a comment like # type: ignore
(IMO cast(...) is more useful for values within a function - from the point on, the type is clear)
| # Attach route without using decorator to avoid untyped-decorator when FastAPI types are unavailable | ||
| fastapi_app.get("/search")(search_endpoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really unfortunate if have to use this way to workaround the issue.
What does it mean by "when FastAPI types are unavailable"? Missed a package? If add the package to dependency, will it pass the check?
| CLIP_MODEL_NAME = "openai/clip-vit-large-patch14" | ||
| CLIP_MODEL_DIMENSION = 768 | ||
| CLIP_MODEL_DIMENSION: Final[int] = 768 | ||
| CLIPVector: TypeAlias = cocoindex.Vector[cocoindex.Float32, Literal[768]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change, seems CLIP_MODEL_DIMENSION is no longer used. I think we can remove that?
| CLIP_MODEL_NAME = "openai/clip-vit-large-patch14" | ||
| CLIP_MODEL_DIMENSION = 768 | ||
| ClipVectorType = cocoindex.Vector[cocoindex.Float32, Literal[CLIP_MODEL_DIMENSION]] | ||
| CLIP_MODEL_DIMENSION: Final = 768 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, looks like CLIP_MODEL_DIMENSION is never used now?
• Make all example entry points pass mypy when run individually (with --ignore-missing-imports).
• Add a small helper script (dev/mypy_check_examples.ps1) and a brief note in dev/README.md.
Closes #1091