Skip to content

Improved Type Safety in Python Client API#42

Open
Itssshikhar wants to merge 4 commits into
kscalelabs:masterfrom
Itssshikhar:typed-dict-api
Open

Improved Type Safety in Python Client API#42
Itssshikhar wants to merge 4 commits into
kscalelabs:masterfrom
Itssshikhar:typed-dict-api

Conversation

@Itssshikhar
Copy link
Copy Markdown

Description

This PR improves the type safety of the Python client API by ensuring consistent TypedDict usage across all service clients. This makes the API more predictable.

Changes

  • Verified and maintained proper TypedDict definitions across all service clients
  • Ensured consistent typing patterns across the codebase
  • Fixed linting issues to maintain code quality
  • Verified all tests pass with the typing improvements

Testing

  • ✅ Ran make format for code formatting
  • ✅ Fixed all linting issues with ruff
  • ✅ Ran make test to verify functionality
  • ✅ Verified type checking with mypy

Related Issues

Closes #32

@codekansas
Copy link
Copy Markdown
Member

awesome :) could you fix the failing unit tests? otherwise looks good!

@Itssshikhar
Copy link
Copy Markdown
Author

thanks @codekansas . I just fixed the failing unit tests and pushed it.

Copy link
Copy Markdown
Contributor

@WT-MM WT-MM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, this looks really good! Just a few nitpicks to satisfy my ocd

Comment thread kos-py/pykos/services/actuator.py Outdated
Comment thread kos-py/pykos/services/actuator.py Outdated
Comment thread kos-py/pykos/services/actuator.py Outdated
@Itssshikhar
Copy link
Copy Markdown
Author

@WT-MM Hope this helps :)

@WT-MM
Copy link
Copy Markdown
Contributor

WT-MM commented Jan 22, 2025

okay, actually after taking another look at this again, can we keep the protobuf return types if they're typed or use dataclasses?

Being able to access values as attributes is probably more ergonomic (e.g. getting response.states instead of response["states'])

Otherwise, it's probably ready to merge in - I'll resolve conflicts and merge in afterwards

@Itssshikhar
Copy link
Copy Markdown
Author

@WT-MM you are right. Having access to the values as attributes is better.
I'll take a look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't use Any type in API

3 participants