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

Type hints napalm.base and napalm.nxos #1412

Merged
merged 16 commits into from
Apr 30, 2021

Conversation

Kircheneer
Copy link
Contributor

I have now gone ahead and put type hints in for napalm.base and napalm.nxos. I am aware that this is a lot of change for a single pull request, as a good amount of code to be executed at runtime has been added as well (lots of assert isinstance(...)). I wanted to make sure that all of the typing errors are gone before I submitted the PR because only then I can know that the whole type hint construct is more or less well defined. If I was still missing bits, other - already annotated - bits might not type check correctly. While I have made an effort as to not cause any regressions through these runtime behavior changes I still think that this PR needs a rather thorough review due to this. I'm more than happy to have a discussion here.

I have also added Mypy to the GitHub Actions workflow file.

There is a final discussio point about a type error which I have currently silenced (mypy ran without the # type: ignore in this snippet).

$ mypy -p napalm
napalm\base\helpers.py:304: error: Incompatible default for argument "default" (default has type "str", argument has type "R")
Found 1 error in 1 file (checked 58 source files)

I have put generic type variables in for the convert function as follows:

def convert(to: Callable[[T], R], who: Optional[T], default: R = "") -> R:  # type: ignore
    ...

Mypy complains about this because "" is not of type R. I held off on changing this due to the ~200 occurences of this function within the various drivers as I did not want to change anything in any drivers but the NXOS and the base one. My proposed solution would be to go through and make sure a default is passed for every call.

Copy link
Member

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

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

Nice work. I think I like the approach.

Any thoughts on this @ktbyers?

@mirceaulinic mirceaulinic requested a review from ktbyers April 1, 2021 10:06
@mirceaulinic mirceaulinic merged commit ab188ee into napalm-automation:develop Apr 30, 2021
@mirceaulinic mirceaulinic modified the milestones: 3.3.0, 4.0.0 May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants