Type hinting and casts

[Kircheneer]

I'm working on type hinting the NXOS drivers at the moment and I'm now faced with a decision. In load_merge_candidate for example there is a piece of code (see below) in which both parameters are Optional (can be None). The first if statement (logically) makes sure, that one of them is not None. Mypy itself does not realize this. Now there's a couple of ways to go about this:

1. Use typing.cast as a hint to the type checker (used as a statement in the actual code, isn't run at runtime)
2. Use assert isinstance(...)
3. Use # type: ignore on the affected lines.

I wanted to clarify as to which would be preferred. 2 has actual runtime consequences, while 1 and 3 don't. 3 is the least "invasive" to the look of the code in my opinion, but 1 makes for a much easier time in writing the type hints, because it "sticks" to the variable (as opposed to #type: ignore)

What's everyone's opinion on this?

def load_merge_candidate(
        self, filename: Optional[str] = None, config: Optional[str] = None
    ) -> None:
        if not filename and not config:
            raise MergeConfigException("filename or config param must be provided.")

        self.merge_candidate += "\n"  # insert one extra line
        if filename is not None:
            with open(filename, "r") as f:
                self.merge_candidate += f.read()
        else:
            self.merge_candidate += config
        self.replace = False
        self.loaded = True

[ktbyers]

@Kircheneer Can you post the mypy message about this line that is failing?

[Kircheneer]

Actually that example does not raise any errors, I made a mistake there. The following does, though:

    def load_merge_candidate(
        self, filename: Optional[str] = None, config: Optional[str] = None
    ) -> None:
        if not filename and not config:
            raise MergeConfigException("filename or config param must be provided.")

        self.merge_candidate += "\n"  # insert one extra line
        if filename is not None:
            with open(filename, "r") as f:
                self.merge_candidate += f.read()
        else:
            self.merge_candidate += config
        self.replace = False
        self.loaded = True

Line 145 is self.merge_candidate += config.

napalm/nxos/nxos.py:156: error: Unsupported operand types for + ("str" and "None")
napalm/nxos/nxos.py:156: note: Right operand is of type "Optional[str]"

[ktbyers]

If the code clearly indicates it must be a string or else it would fail, I would just add an assert isinstance(some_var, str) before the relevant line.

[Kircheneer]

Alright, that's what I'll be doing then. Thanks!

[Kircheneer]

@ktbyers I have found another case where this is interesting. Lots of methods in the NXOS code like the below method. I can't really go and isinstance these because that breaks the tests. Maybe NetworkDriver.device could become a @property at some point so open() is called automatically whenever anything that needs it is accessed?

def _send_command(
    self, command: str, raw_text: bool = False
) -> Dict[str, Union[str, Dict[str, Any]]]:
    """
    Wrapper for NX-API show method.

    Allows more code sharing between NX-API and SSH.
    """
    return self.device.show(command, raw_text=raw_text)

The error Mypy throws for this is the following:

napalm\nxos\nxos.py:767: error: Item "None" of "Optional[Device]" has no attribute "show"

[Kircheneer]

I solved this with assert self.device is not None in those specific methods for #1412