Repository: enowars/enochecker

Issue:
Show warning when returning error status without accompanying message
As mentioned in https://github.com/enowars/enochecker/pull/55 while returning Result.MUMBLE or Result.OFFLINE is supported it does not include details which would be necessary for e.g. https://github.com/enowars/enowars4-scoreboard/issues/2

The preferred and documented way should be using Exceptions instead

Discussion:
domenukk: Why not deprecate all returns? 
ldruschk: That would also work, but unless we enforce the type annotations in the checkers and change the return type from the functions from `Optional[Result]` to `None`, there is no difference in returning a `Result.OK` that is discarded or returning nothing at all.
domenukk: My gut feeling is that we should either do it completely (deprecate all returns now, change the return type to `none` and remove the handling in a few months or so) or not deprecate it at all. Allowing exactly one return seems odd.
domenukk: But anyway it's also fine to allow and ignore it, just odd

Patch:
diff --git a/src/enochecker/enochecker.py b/src/enochecker/enochecker.py
index 831cb5b..0e0e4ca 100644
--- a/src/enochecker/enochecker.py
+++ b/src/enochecker/enochecker.py
@@ -455,6 +455,11 @@ class BaseChecker(metaclass=_CheckerMeta):
 
                 # Better wrap this, in case somebody returns raw ints (?)
                 ret = Result(ret)
+                if ret != Result.OK:
+                    warnings.warn(
+                        "Returning a non-ok status code is not recommended and will be removed in the future. Raise an Exception with additional text instead.",
+                        DeprecationWarning,
+                    )
                 self.info("Checker [{}] resulted in {}".format(self.method, ret.name))
                 self.team_db[
                     f"__Checker-internals-RESULT:{str(method)},{self.flag_round},{self.flag_idx}__"


Test patch:
diff --git a/tests/test_run_method.py b/tests/test_run_method.py
index d059e30..ecaa6af 100644
--- a/tests/test_run_method.py
+++ b/tests/test_run_method.py
@@ -101,7 +101,10 @@ def test_run_return_status(method, result, checker_cls):
 
     setattr(checker_cls, method, meth)
     c = checker_cls(method)
-    res = c.run()
+    with pytest.warns(
+        None
+    ):  # ignore warnings caused by the deprecation of returning Results
+        res = c.run()
     assert isinstance(res, CheckerResult)
     assert res.result == result
 
@@ -293,3 +296,59 @@ def test_offline_exceptions(method, exc, checker_cls):
     assert isinstance(res, CheckerResult)
     assert res.result == Result.OFFLINE
     assert res.message
+
+
+@pytest.mark.parametrize(
+    "method", CHECKER_METHODS,
+)
+def test_no_warn_deprecated_return_ok(method, checker_cls):
+    def meth(self):
+        return Result.OK
+
+    def ok(self):
+        return Result.OK
+
+    if method == "getflag":
+        setattr(checker_cls, "putflag", ok)
+        c = checker_cls("putflag")
+        c.run()
+    elif method == "getnoise":
+        setattr(checker_cls, "putnoise", ok)
+        c = checker_cls("putnoise")
+        c.run()
+
+    setattr(checker_cls, method, meth)
+    c = checker_cls(method)
+    with pytest.warns(None) as record:
+        res = c.run()
+    assert isinstance(res, CheckerResult)
+    assert res.result == Result.OK
+    assert len(record) == 0
+
+
+@pytest.mark.parametrize(
+    "method, ret",
+    product(CHECKER_METHODS, [Result.MUMBLE, Result.OFFLINE, Result.INTERNAL_ERROR]),
+)
+def test_warn_deprecated_return_value(method, ret, checker_cls):
+    def meth(self):
+        return ret
+
+    def ok(self):
+        return Result.OK
+
+    if method == "getflag":
+        setattr(checker_cls, "putflag", ok)
+        c = checker_cls("putflag")
+        c.run()
+    elif method == "getnoise":
+        setattr(checker_cls, "putnoise", ok)
+        c = checker_cls("putnoise")
+        c.run()
+
+    setattr(checker_cls, method, meth)
+    c = checker_cls(method)
+    with pytest.deprecated_call():
+        res = c.run()
+    assert isinstance(res, CheckerResult)
+    assert res.result == ret