test utility refactoring for time/fuzzing wrappers of checkPythonCommand

coderabbitai[bot] commented 1 month ago

Summary

In the discussion on PR #127 (https://github.com/reactive-firewall/multicast/pull/127) (see comment https://github.com/reactive-firewall/multicast/pull/127#discussion_r1774288385 by @reactive-firewall), we agreed to refactor the timePythonCommand and checkPythonFuzzing functions to use *args and keyword-only arguments. This change avoids mutable default arguments, aligns with the intended functionality, and improves debugging.

Plan for Next Steps

Refactoring:
- Update the timePythonCommand and checkPythonFuzzing functions to use *args and keyword-only arguments (stderr=None).
- Modify the functions to raise a RuntimeError if args is empty or the first argument is None.
- Ensure that checkPythonCommand can handle args as a tuple or convert it to a list if necessary.
Testing:
- Write new unit tests to cover the updated functions, including tests for the new exception handling when args is empty or contains None.
- Run the test suite to ensure that all tests pass with the refactored code.
Documentation:
- Update the docstrings of the refactored functions to reflect the new parameter structure and behavior.
- Document any changes in the project's documentation, highlighting the refactoring and its benefits.

References

PR #127: https://github.com/reactive-firewall/multicast/pull/127
Comment: https://github.com/reactive-firewall/multicast/pull/127#discussion_r1774288385 by @reactive-firewall

reactive-firewall commented 4 weeks ago

Python 3 Compatibility:

Supported Syntax: In Python 3.x, this syntax is fully supported. You can define functions with variable positional arguments (*args) followed by keyword-only arguments with default values. This allows you to enforce that certain arguments must be provided as keyword arguments.

Official Documentation:

Python 3 Documentation - Function Definitions: This section explains the syntax of function definitions, including the use of *args and keyword-only arguments.

PEP 3102 – Keyword-Only Arguments: This Python Enhancement Proposal introduced keyword-only arguments in Python 3.0, allowing arguments following *args to be specified as keyword-only.

Python 2 Compatibility:

Syntax Limitation: In Python 2.x, the syntax def someFunction(*args, optional_non_positional_arg=None): is not supported. Placing a parameter after *args without using **kwargs will result in a SyntaxError.

_Originally posted by @coderabbitai[bot] in https://github.com/reactive-firewall/multicast/pull/127#discussion_r1774331992_

Basically we want to go with:

def timePythonCommand(*args, stderr=None):
    if not args or args[0] is None:
        raise RuntimeError("args must be an array of positional arguments")
    return checkPythonCommand(args, stderr)

and

def checkPythonFuzzing(*args, stderr=None):
    if not args or args[0] is None:
        raise RuntimeError("args must be an array of positional arguments")
    return checkPythonCommand(args, stderr)

and see #128

def checkCovCommand(*args):
    ... docstrings go here ...
    if not args or args[0] is None:
        raise RuntimeError("args must be an array of positional arguments")
    elif str("coverage") in args[0]:
    ... rest of function

coderabbitai[bot] commented 4 weeks ago

[!TIP] For best results, initiate chat on the files or code changes.

@reactive-firewall, thank you for the clarification and the examples provided. I see that the refactoring aligns with the objectives outlined in PR #127 and the follow-up in #128. The use of *args and keyword-only arguments (stderr=None) is a great approach to ensure flexibility while enforcing the necessary checks for positional arguments.

I'll proceed with the following:

Refactoring:
- Implement the timePythonCommand, checkPythonFuzzing, and checkCovCommand functions as per your examples.
Testing:
- I'll ensure that the unit tests cover the new exception handling for empty or None arguments.
Documentation:
- I'll update the docstrings to reflect the new parameter structure and behavior.

Let me know if you'd like me to open a new issue for tracking this refactor or if you'd prefer to handle it within the current PR.

Also, I'll make sure to retain the learning about your preference for using *args and raising exceptions for missing arguments instead of using args=None.

✏️ Learnings added

``` Learnt from: reactive-firewall URL: https://github.com/reactive-firewall/multicast/issues/129 Timestamp: 2024-09-28T00:41:04.876Z Learning: Reactive-firewall prefers using `*args` and raising exceptions for missing arguments instead of using `args=None` in function definitions. ``` > Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure [review instructions](https://docs.coderabbit.ai/guides/review-instructions). You can manage existing learnings in the [app](https://app.coderabbit.ai/learnings).

🧠 Learnings used

``` Learnt from: reactive-firewall PR: reactive-firewall/multicast#110 File: docs/CI.md:32-32 Timestamp: 2024-09-17T06:40:42.918Z Learning: Reactive-firewall prefers to keep the word "own" in headings when emphasizing the reader's personal test. ``` --- ``` Learnt from: reactive-firewall PR: reactive-firewall/multicast#110 File: docs/USAGE.md:66-66 Timestamp: 2024-09-17T03:38:28.677Z Learning: Reactive-firewall prefers to use "Caveat" as a heading instead of "Caution". ``` --- ``` Learnt from: reactive-firewall PR: reactive-firewall/multicast#118 File: tests/check_pip:92-197 Timestamp: 2024-09-24T01:03:10.574Z Learning: Reactive-firewall prefers defining functions before they are called in Bash scripts to avoid errors due to Bash's sequential interpretation. ``` --- ``` Learnt from: reactive-firewall PR: reactive-firewall/multicast#127 File: tests/context.py:485-490 Timestamp: 2024-09-25T03:13:33.446Z Learning: Reactive-firewall prefers using `*args` and raising exceptions for missing arguments instead of using `args=None` in function definitions. ```

reactive-firewall / multicast