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

Add more types to network_lite #398

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

dhdaines
Copy link
Collaborator

More types! More types!

Copy link
Contributor

github-actions bot commented Sep 11, 2024

CLI load time: 0:00.05
Pull Request HEAD: cd61c425df891db3c671afe438e3b750fce10736
Imports that take more than 0.1 s:
import time: self [us] | cumulative | imported package

@joanise joanise changed the base branch from main to dev.ej/network-lite September 11, 2024 19:05
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.50%. Comparing base (b5e2258) to head (1b5573f).

Additional details and impacted files
@@                   Coverage Diff                   @@
##           dev.ej/network-lite     #398      +/-   ##
=======================================================
+ Coverage                93.46%   93.50%   +0.03%     
=======================================================
  Files                       18       18              
  Lines                     2541     2554      +13     
  Branches                   576      576              
=======================================================
+ Hits                      2375     2388      +13     
  Misses                      95       95              
  Partials                    71       71              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joanise
Copy link
Collaborator

joanise commented Sep 11, 2024

Wow, that is some serious typing work! Thanks for doing this, I would not have known how to do it all.
Maybe add one more:

LANGS_NETWORK: DiGraph[str] = load_network()

in langs/__init__.py

@dhdaines
Copy link
Collaborator Author

dhdaines commented Sep 11, 2024

One thing to note here: I'm using (with some difficulty) TypedDict to check the types for node_link_graph and node_link_data but this is actually not very useful because they deal with data that is loaded from JSON at runtime and thus always has type Any. So, in fact, we should use Pydantic here instead ;-)

I shouldn't take any more time away from my other important work (whatever it is) to do that today but I might do it later this week.

@dhdaines
Copy link
Collaborator Author

One thing to note here: I'm using (with some difficulty) TypedDict to check the types for node_link_graph and node_link_data but this is actually not very useful because they deal with data that is loaded from JSON at runtime and thus always has type Any.

Run-time validation is really a separate issue here (and not strictly necessary given that we know where the data comes from) so I'll leave it for the future.

This commit combines the followed commits, all by dhd:
 - feat: add full type signatures to DiGraph
 - fix: support python 3.7
 - fix: one more type annotation
   Theres always one more type annotation!
 - fix: one more type annotation (again!)
   Theres always one more type annotation!
 - fix: nope! it does not just work
 - fix: one more type annotation
   Theres always one more type annotation!
Copy link
Collaborator

@joanise joanise left a comment

Choose a reason for hiding this comment

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

This is awesome, thank you!

@joanise joanise merged commit cd61c42 into dev.ej/network-lite Sep 12, 2024
6 checks passed
@joanise joanise deleted the dev.dhd/network-lite-types branch September 12, 2024 13:21
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