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 hinting #1476

Merged
merged 46 commits into from
Feb 12, 2022
Merged

Type hinting #1476

merged 46 commits into from
Feb 12, 2022

Conversation

Kircheneer
Copy link
Contributor

Something weird happened with #1434 so I redid the PR - again. This should be mostly free of any non-sense commits and should reflect a proper and useful implementation of type hinting for the NXOS parts of NAPALM.

@Kircheneer
Copy link
Contributor Author

Anything else I can do here to help get this merged?

@ktbyers
Copy link
Contributor

ktbyers commented Nov 26, 2021

Yeah, don't hold this up for me...so if it is good with @mirceaulinic (then it is fine with me).

@mirceaulinic
Copy link
Member

Hi @Kircheneer. Yes, just let me release version 3.4.0 (hopefully next week), then we can merge this and start preparing release 4.0.0. Sorry for the wait!

@mirceaulinic mirceaulinic added this to the 4.0.0 milestone Nov 29, 2021
@Kircheneer
Copy link
Contributor Author

Thanks @mirceaulinic. Just let me know whenever we are ready for merging, then I'll clean up the conflicting files again.

@Kircheneer
Copy link
Contributor Author

Left with the following errors:

napalm/base/helpers.py:305: error: Incompatible default for argument "default" (default has type "str", argument has type "R")
napalm/base/mock.py:201: error: Module has no attribute "CommitError"

For the first one: The docs state explicitly that a str will always be returned, even though in general I though this would be a generic function. Any clues as to what degree this is true?

For the second one: I'm unsure how this got introduced - any pointers?

@Kircheneer
Copy link
Contributor Author

Regarding the CommitError thing, I'm thinking that this is probably a bug Mypy uncovered. I just moved the raise napalm.CommitError("Pending commit confirm already in process!") line to the front of the commit_config method:

    def commit_config(self, message: str = "", revert_in: Optional[int] = None) -> None:

        raise napalm.CommitError("Pending commit confirm already in process!")
        count = self._count_calls("commit_config")
        self._raise_if_closed()
        if revert_in is not None:
            if self.has_pending_commit():
                raise napalm.CommitError("Pending commit confirm already in process!")
            else:
                self._pending_commits = True
        self.merge = None
        self.filename = None
        self.config = None
        mocked_data(self.path, "commit_config", count)

This lead to the following unit test errors:

FAILED test/base/test_mock_driver.py::TestMockDriver::test_configuration_merge - AttributeError: module 'napalm' has no attribute 'CommitError'
FAILED test/base/test_mock_driver.py::TestMockDriver::test_configuration_replace - AttributeError: module 'napalm' has no attribute 'CommitError'
FAILED test/base/test_mock_driver.py::TestMockDriver::test_configuration_replace_confirm - AttributeError: module 'napalm' has no attribute 'CommitError'

I'll put in a fix for that into this branch if you don't mind.

About the other issue I think we need to have a synchronous discussion because that touches lots of places at once.

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.

Awesome work, @Kircheneer! 🙇🏼

@mirceaulinic mirceaulinic merged commit e062fc5 into napalm-automation:develop Feb 12, 2022
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.

3 participants