Skip to content

Add type hints to LayeredActiveSpace#91

Open
arpitjain099 wants to merge 2 commits into
dbetchkal:mainfrom
arpitjain099:chore/type-hints-layered-active-space
Open

Add type hints to LayeredActiveSpace#91
arpitjain099 wants to merge 2 commits into
dbetchkal:mainfrom
arpitjain099:chore/type-hints-layered-active-space

Conversation

@arpitjain099

Copy link
Copy Markdown
Contributor

Adds type annotations to all methods and attributes in layered_active_space.py. Matches the annotation style already used in computation.py, helpers.py, and models.py. No logic changes.

Annotate all method signatures and instance attributes in the
LayeredActiveSpace class, matching the typing style already used
in computation.py, helpers.py, and models.py.

Signed-off-by: arpitjain099 <arpitjain099@gmail.com>

@elliott-ruebush elliott-ruebush left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like adding type hints to more functions! It's a good quality of life/code style improvement and should make future dev work easier.

Can we use the most up to date style of type hints when adding new ones though? I will look to get together a style guide and include that as part of the CONTRIBUTING.md PR

@@ -1,3 +1,5 @@
from typing import Dict, Optional

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this repo is on python3.12 - could we use a more modern type hint style? e.g. dict[<types>] instead of Dict[<types>] and <type> | None instead of Optional[<type>]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will update to use the 3.12+ style throughout. Pushing a fix now.

Replace Dict/Optional from typing with builtin dict and X | None
union syntax per reviewer feedback.

Signed-off-by: arpitjain099 <arpitjain099@gmail.com>
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.

2 participants