-
Notifications
You must be signed in to change notification settings - Fork 12
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 type hints for parameters and return #189
Conversation
805648d
to
aa5bbd5
Compare
@putnopvut @LorenzoBianconi @numansiddique what do you think of this change? |
This PR made me learn what type hints are. I've no objections to it. If this PR is accepted, I'd suggest make it a norm (or must) for new PRs to follow type hinting. Thanks |
aa5bbd5
to
278afcf
Compare
Good point, there actually are ways to enforce this. One way is to run |
ovn-tester/ovn_tester.py
Outdated
def read_physical_deployment(deployment, global_cfg): | ||
def read_physical_deployment( | ||
deployment: str, global_cfg: GlobalCfg | ||
) -> Tuple[PhysicalNode, List[PhysicalNode]]: |
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 actually returns Tuple[List[PhysicalNode], List[PhysicalNode]]
.
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.
done
Signed-off-by: jishi <[email protected]>
278afcf
to
ab55908
Compare
It makes sense, however if we want to check those rules it should be part of the PR so there is no chance to "break" in the meantime. |
Sure, that's the ideal way. However, I think it's a pity to delay this PR for that. I opened an issue to track running and enforcing rules with mypy on new PRs: #192 |
I've run following tests with the patch:
./do.sh run test-scenarios/ovn-20.yml ovn-20
./do.sh run test-scenarios/ovn-low-scale.yml ovn-low-scale
./do.sh run test-scenarios/ovn-low-scale-ic.yml ovn-low-scale-ic