Skip to content
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

Fix type errors caught by pyright but not mypy #156

Merged
merged 17 commits into from
Jul 24, 2023
Merged

Conversation

tpoliaw
Copy link
Collaborator

@tpoliaw tpoliaw commented Jul 13, 2023

  • Use type as type of Inputs/Outputs fields
  • Use correct types instead of string literals in tests
  • Use Iterator instead of Iterable as the return type when wrapping
  • Check optional keys are present before access
  • Ignore type checking of dynamic softioc dbl method
  • Strengthen type handling of strings vs bytes in interpreters/adapters

Where type checking has been ignored, comments have been added to explain
why the code is still valid.

@tpoliaw tpoliaw changed the base branch from master to typed_dict_typing July 13, 2023 16:35
@tpoliaw tpoliaw changed the base branch from typed_dict_typing to master July 13, 2023 16:35
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #156 (a7dc12d) into master (6066902) will decrease coverage by 0.08%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
- Coverage   94.50%   94.42%   -0.08%     
==========================================
  Files          44       44              
  Lines        1292     1310      +18     
==========================================
+ Hits         1221     1237      +16     
- Misses         71       73       +2     
Impacted Files Coverage Δ
src/tickit/adapters/epicsadapter/ioc_manager.py 59.37% <0.00%> (ø)
src/tickit/core/state_interfaces/kafka.py 83.33% <0.00%> (-2.88%) ⬇️
src/tickit/utils/configuration/tagged_union.py 87.23% <ø> (ø)
...kit/adapters/interpreters/command/regex_command.py 96.15% <94.44%> (-3.85%) ⬇️
src/tickit/adapters/epicsadapter/adapter.py 100.00% <100.00%> (ø)
...apters/interpreters/command/command_interpreter.py 96.77% <100.00%> (+0.10%) ⬆️
src/tickit/adapters/interpreters/utils.py 100.00% <100.00%> (ø)
...pters/interpreters/wrappers/joining_interpreter.py 100.00% <100.00%> (ø)
...ers/interpreters/wrappers/splitting_interpreter.py 100.00% <100.00%> (ø)
...rc/tickit/core/state_interfaces/state_interface.py 88.23% <100.00%> (ø)
... and 3 more

@tpoliaw tpoliaw force-pushed the pyright_errors branch 2 times, most recently from 1b3ff04 to 5271a2b Compare July 14, 2023 11:06
Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

Looks good, only a few minor comments.

src/tickit/adapters/epicsadapter/adapter.py Show resolved Hide resolved
examples/devices/zeromq_push_device.py Show resolved Hide resolved
@@ -21,7 +21,7 @@ def __init__(
self,
host: str = "127.0.0.1",
port: int = 5555,
socket_factory: Optional[SocketFactory] = create_zmq_push_socket,
socket_factory: SocketFactory = create_zmq_push_socket,
Copy link
Contributor

Choose a reason for hiding this comment

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

Good spot, should probably also do this in the base class

@tpoliaw
Copy link
Collaborator Author

tpoliaw commented Jul 14, 2023

There are still a few (6) errors remaining but mostly around the interpreters which I'd like to overhaul slightly anyway

The class variables are types rather than instances in their own right
so should be typed as such. This allows type checkers to correctly check
key lookups.
Iterators are iterable but the more specific type allows callers to
use additional methods.
Methods are generated dynamically so static type checking can't work.
@tpoliaw tpoliaw changed the title pyright errors Fix type errors caught by pyright but not mypy Jul 21, 2023
@tpoliaw tpoliaw requested a review from abbiemery July 21, 2023 13:20
Copy link
Collaborator

@abbiemery abbiemery left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for that. Once the zeromq_push_device comment has been fixed I think you're good to go. Do you want me to change the mypy to pyright in the checks in here, or should we do a different pr for it?

@tpoliaw
Copy link
Collaborator Author

tpoliaw commented Jul 24, 2023

Up to you but I think I'd go for a separate PR so it can be rolled back more easily if there's a pyright problem we can't work around. Everything in this PR should still be valid if we stick with mypy.

@abbiemery
Copy link
Collaborator

Agree. In which case either resolve the issue in the push device or make a new issue for it, and merge this :)

@tpoliaw tpoliaw merged commit 8529b9c into master Jul 24, 2023
16 checks passed
@tpoliaw
Copy link
Collaborator Author

tpoliaw commented Jul 24, 2023

make a new issue for it

#170

@abbiemery abbiemery deleted the pyright_errors branch July 25, 2023 11:25
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.

3 participants