A small logic error and several redundant type-checking conditional statements #68461
Description
While reviewing /salt/utils/path.py, I found an unreachable branch in the function sanitize_win_path(winpath). It seems like a logic error caused by a typo, but I am not sure whether the conditional statement should be elif not isinstance(winpath, str): (else: is better in my opinion) or was supposed to check other types.
def sanitize_win_path(winpath):
"""
Remove illegal path characters for windows
"""
intab = "<>:|?*"
if isinstance(winpath, str):
winpath = winpath.translate({ord(c): "_" for c in intab})
elif isinstance(winpath, str): # unreachable branch, dead code
outtab = "_" * len(intab)
trantab = "".maketrans(intab, outtab)
winpath = winpath.translate(trantab)
return winpath
Besides, there are several redundant type checks that can be removed logically. For example, in the function check_result(...) of /salt/utils/state.py, the inner isinstance(state_result, dict) must be True due to the outer type check.
if ret and isinstance(state_result, dict):
result = state_result.get("result", _empty)
if result is False:
ret = False
# only override return value if we are not already failed
elif result is _empty and isinstance(state_result, dict) and ret: # isinstance(state_result, dict) must be True
ret = check_result(state_result, recurse=True, highstate=highstate)
Another recurring scenario is that some if-elif branches can be simplified to if-else. For example, two redundant checks from /salt/modules/tls.py as follows and six similar checks from /salt/states/cloud.py. However, they are not in the master branch. I am just worried that this coding style might easily lead to typo.
def _check_onlyif_unless(onlyif, unless):
ret = None
retcode = __salt__["cmd.retcode"]
if onlyif is not None:
if not isinstance(onlyif, str):
if not onlyif:
ret = {"comment": "onlyif condition is false", "result": True}
elif isinstance(onlyif, str): # -> else:
if retcode(onlyif) != 0:
ret = {"comment": "onlyif condition is false", "result": True}
log.debug("onlyif condition is false")
if unless is not None:
if not isinstance(unless, str):
if unless:
ret = {"comment": "unless condition is true", "result": True}
elif isinstance(unless, str): # -> else:
if retcode(unless) == 0:
ret = {"comment": "unless condition is true", "result": True}
log.debug("unless condition is true")
return ret