janware/jw-pkg
Watch
1
Code Pull requests Activity Actions

lib.App: Refactor App / Cmd to adress multiple functionality and style issues #60

Merged
Jan Lindemann merged 12 commits from jan/fix/20260815-lib-app-cmd-refactor-multiple-functionality-and-style-issues into master 2026-08-15 16:07:54 +02:00 AGit
Conversation 0 Commits 12 Files changed 2 +140 -76
Jan Lindemann commented 2026-08-15 15:58:48 +02:00
Owner
Copy link

This PR fixes miscellaneous bug and code smell issues in jw.pkg.lib.App / jw.pkg.lib.Cmd. As far as the bugs are concerned: No unit tests exist for the respective cases, they had not been used by downstream code, and they have been dug up by AI code analysis.

lib.App: Fix crash on invalid log options

The --log-level and --log-flags options are added without a type converter, so argparse never validates their values. _build_parser() then hands the raw string to set_log_level() and set_log_flags() during its first parse, and an unparseable value such as "INVALID" crashes init() with a raw ValueError traceback instead of a usage error.

Pass type = parse_log_level() and type = parse_log_flags() when adding the options, so that argparse reports invalid values with the standard usage error and exit status 2. argparse only applies the converter to command-line strings, so the int and LogFlag defaults are unaffected.

lib.App: Implement aenter() and aexit()

aenter() and aexit() are empty stubs. Using the application as an async context manager therefore binds None in the as clause, and releases nothing on exit: an AsyncRunner created through call_async() keeps running in its thread, and since that thread is not a daemon, the process does not exit after the block.

Return self from aenter(), and call close() from aexit(), so that the context manager binds the application and releases all resources on exit, whether the block exits normally or with an exception.

lib.App: Release async runner in close()

close() closes the application's own event loop, but the AsyncRunner is only released in the finally block of run(). An application that creates a runner through call_async() and then calls close(), for instance through the async context manager, therefore leaks the runner, and close() does not fulfill its contract of releasing all resources.

Move the AsyncRunner cleanup from the finally block of run() into close() and reset the own-loop flag when the loop is closed, so that close() releases everything and run() only has to call it.

lib.App: Restore previous event loop in run()

When run() creates an event loop, it installs it with set_event_loop() but never restores the thread's previous loop, so after run() returns, the thread is left with the now-closed loop created by run(). Any code that calls get_event_loop() afterwards gets a closed loop, and on Python 3.13+ a thread that had no loop at all starts emitting or raising deprecation errors that run() caused.

Capture the thread's current loop with _get_current_event_loop() before installing a new one, and restore it in the finally block. If there was no previous loop, unset the loop with set_event_loop(None) so that the thread is left without a loop instead of with the closed one.

lib.App: Rebuild parser from run() argv

init() builds the parser and the lazy subcommand registration inside it decides which subcommands to register by re-parsing sys.argv. run() then parses a different argv, so if the caller passes an argv that is deeper than the one in sys.argv, the required subparsers have not been registered and the invocation fails with an "unrecognized arguments" error. run_sub_commands() passes argv to run(), so the mismatch is reachable from the public API.

Move the parser construction from init() into _build_parser() and call it from run() when an argv is given, so that registration and parsing are driven by the same command line. The top-level command instances are created once in init() and reused when the parser is rebuilt.

lib.App: Use args parameter in _run()

_run() receives the parsed arguments as its args parameter, but then checks the private __args attribute for the func attribute and resolves the command function through the args property. Both refer to the same object today, so the mixing is harmless, but it obscures the data flow and would silently diverge if a caller ever passed a namespace other than the stored one.

Use the args parameter consistently in _run().

lib.App: Use parser in _add_arguments()

_add_arguments() adds the global options to self.__parser instead of to the parser it receives. The two are the same object, because the only caller passes self.__parser, so the change is not observable.

Use the parser parameter instead, so that the method honors its argument the way Cmd.add_arguments() does, and so that the global options can be shared with other parsers, e.g. the subcommand parsers, without rewriting this method.

lib.App: Tidy up subcommand registration

The subcommand registration in _build_parser() defines a SubCommand helper class inside the add_cmds_to_parser() closure, so a fresh class object is created on every call. It also stores command names and aliases in a dictionary without checking for duplicates, so a colliding name or alias is silently overwritten, and it relies on every subparser level sharing the dest = 'command' attribute to descend one level per re-parse, an invariant that is not documented anywhere.

Hoist the helper to a module-level _SubCommand NamedTuple, log a warning when a subcommand name or alias collides with an earlier one at the same level, and document the dest = 'command' invariant next to the re-parse.

lib.App: Warn on invalid exit status

__run() only accepts a return value from _run() as the process exit status if it is an int between 0 and 255, and silently drops any other value. A command that returns, for instance, 300 therefore exits with status 0, which presents a failure as a success to the caller without any trace of the mistake.

Log an error when the returned exit status is out of range so that the programming error is visible, while still exiting with 0 instead of passing an invalid status to the shell.

lib.App: Log reason for skipped completion

The shell completion setup in __run() catches every exception and silently ignores it. If argcomplete is missing or its initialization fails, the completion is simply not available and there is no trace of why, even when logging is turned up to debug level.

Log the reason at debug level: one message when the argcomplete import fails and one with the exception for any other failure.

lib.App: Pass None default to os.getenv()

The os.getenv() calls in init() that read the log and backtrace defaults mostly pass None as the explicit default value, but the one for the show-backtrace environment variable relies on the implicit default.

Pass None explicitly there as well, so that all the calls read the same way.

lib.Cmd: Add single Cmd to add_subcommands()

add_subcommands() advertises Cmd and list[Cmd] in its signature, but a single Cmd instance raises NotImplementedError, and since the list branch handles every element through the same method, a list of Cmd instances is broken as well. The only working forms are Types and lists of Types.

Handle a single Cmd instance by reparenting it to the caller and appending it to the children, tracking its class like the class-based path does. Instances whose name is already taken by a child are rejected, mirroring the duplicate-class handling, because argparse cannot register two subparsers under the same name.

This PR fixes miscellaneous bug and code smell issues in jw.pkg.lib.App / jw.pkg.lib.Cmd. As far as the bugs are concerned: No unit tests exist for the respective cases, they had not been used by downstream code, and they have been dug up by AI code analysis. #### lib.App: Fix crash on invalid log options The --log-level and --log-flags options are added without a type converter, so argparse never validates their values. _build_parser() then hands the raw string to set_log_level() and set_log_flags() during its first parse, and an unparseable value such as "INVALID" crashes __init__() with a raw ValueError traceback instead of a usage error. Pass type = parse_log_level() and type = parse_log_flags() when adding the options, so that argparse reports invalid values with the standard usage error and exit status 2. argparse only applies the converter to command-line strings, so the int and LogFlag defaults are unaffected. #### lib.App: Implement __aenter__() and __aexit__() __aenter__() and __aexit__() are empty stubs. Using the application as an async context manager therefore binds None in the as clause, and releases nothing on exit: an AsyncRunner created through call_async() keeps running in its thread, and since that thread is not a daemon, the process does not exit after the block. Return self from __aenter__(), and call close() from __aexit__(), so that the context manager binds the application and releases all resources on exit, whether the block exits normally or with an exception. #### lib.App: Release async runner in close() close() closes the application's own event loop, but the AsyncRunner is only released in the finally block of run(). An application that creates a runner through call_async() and then calls close(), for instance through the async context manager, therefore leaks the runner, and close() does not fulfill its contract of releasing all resources. Move the AsyncRunner cleanup from the finally block of run() into close() and reset the own-loop flag when the loop is closed, so that close() releases everything and run() only has to call it. #### lib.App: Restore previous event loop in run() When run() creates an event loop, it installs it with set_event_loop() but never restores the thread's previous loop, so after run() returns, the thread is left with the now-closed loop created by run(). Any code that calls get_event_loop() afterwards gets a closed loop, and on Python 3.13+ a thread that had no loop at all starts emitting or raising deprecation errors that run() caused. Capture the thread's current loop with _get_current_event_loop() before installing a new one, and restore it in the finally block. If there was no previous loop, unset the loop with set_event_loop(None) so that the thread is left without a loop instead of with the closed one. #### lib.App: Rebuild parser from run() argv __init__() builds the parser and the lazy subcommand registration inside it decides which subcommands to register by re-parsing sys.argv. run() then parses a different argv, so if the caller passes an argv that is deeper than the one in sys.argv, the required subparsers have not been registered and the invocation fails with an "unrecognized arguments" error. run_sub_commands() passes argv to run(), so the mismatch is reachable from the public API. Move the parser construction from __init__() into _build_parser() and call it from run() when an argv is given, so that registration and parsing are driven by the same command line. The top-level command instances are created once in __init__() and reused when the parser is rebuilt. #### lib.App: Use args parameter in _run() _run() receives the parsed arguments as its args parameter, but then checks the private __args attribute for the func attribute and resolves the command function through the args property. Both refer to the same object today, so the mixing is harmless, but it obscures the data flow and would silently diverge if a caller ever passed a namespace other than the stored one. Use the args parameter consistently in _run(). #### lib.App: Use parser in _add_arguments() _add_arguments() adds the global options to self.__parser instead of to the parser it receives. The two are the same object, because the only caller passes self.__parser, so the change is not observable. Use the parser parameter instead, so that the method honors its argument the way Cmd.add_arguments() does, and so that the global options can be shared with other parsers, e.g. the subcommand parsers, without rewriting this method. #### lib.App: Tidy up subcommand registration The subcommand registration in _build_parser() defines a SubCommand helper class inside the add_cmds_to_parser() closure, so a fresh class object is created on every call. It also stores command names and aliases in a dictionary without checking for duplicates, so a colliding name or alias is silently overwritten, and it relies on every subparser level sharing the dest = 'command' attribute to descend one level per re-parse, an invariant that is not documented anywhere. Hoist the helper to a module-level _SubCommand NamedTuple, log a warning when a subcommand name or alias collides with an earlier one at the same level, and document the dest = 'command' invariant next to the re-parse. #### lib.App: Warn on invalid exit status __run() only accepts a return value from _run() as the process exit status if it is an int between 0 and 255, and silently drops any other value. A command that returns, for instance, 300 therefore exits with status 0, which presents a failure as a success to the caller without any trace of the mistake. Log an error when the returned exit status is out of range so that the programming error is visible, while still exiting with 0 instead of passing an invalid status to the shell. #### lib.App: Log reason for skipped completion The shell completion setup in __run() catches every exception and silently ignores it. If argcomplete is missing or its initialization fails, the completion is simply not available and there is no trace of why, even when logging is turned up to debug level. Log the reason at debug level: one message when the argcomplete import fails and one with the exception for any other failure. #### lib.App: Pass None default to os.getenv() The os.getenv() calls in __init__() that read the log and backtrace defaults mostly pass None as the explicit default value, but the one for the show-backtrace environment variable relies on the implicit default. Pass None explicitly there as well, so that all the calls read the same way. #### lib.Cmd: Add single Cmd to add_subcommands() add_subcommands() advertises Cmd and list[Cmd] in its signature, but a single Cmd instance raises NotImplementedError, and since the list branch handles every element through the same method, a list of Cmd instances is broken as well. The only working forms are Types and lists of Types. Handle a single Cmd instance by reparenting it to the caller and appending it to the children, tracking its class like the class-based path does. Instances whose name is already taken by a child are rejected, mirroring the duplicate-class handling, because argparse cannot register two subparsers under the same name.
The --log-level and --log-flags options are added without a type
converter, so argparse never validates their values. _build_parser()
then hands the raw string to set_log_level() and set_log_flags() during
its first parse, and an unparseable value such as "INVALID" crashes
__init__() with a raw ValueError traceback instead of a usage error.

Pass type = parse_log_level() and type = parse_log_flags() when adding
the options, so that argparse reports invalid values with the standard
usage error and exit status 2. argparse only applies the converter to
command-line strings, so the int and LogFlag defaults are unaffected.

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
__aenter__() and __aexit__() are empty stubs. Using the application
as an async context manager therefore binds None in the as clause,
and releases nothing on exit: an AsyncRunner created through
call_async() keeps running in its thread, and since that thread is
not a daemon, the process does not exit after the block.

Return self from __aenter__(), and call close() from __aexit__(), so
that the context manager binds the application and releases all
resources on exit, whether the block exits normally or with an
exception.

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
close() closes the application's own event loop, but the AsyncRunner
is only released in the finally block of run(). An application that
creates a runner through call_async() and then calls close(), for
instance through the async context manager, therefore leaks the
runner, and close() does not fulfill its contract of releasing all
resources.

Move the AsyncRunner cleanup from the finally block of run() into
close() and reset the own-loop flag when the loop is closed, so that
close() releases everything and run() only has to call it.

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
When run() creates an event loop, it installs it with
set_event_loop() but never restores the thread's previous loop, so
after run() returns, the thread is left with the now-closed loop
created by run(). Any code that calls get_event_loop() afterwards
gets a closed loop, and on Python 3.13+ a thread that had no loop at
all starts emitting or raising deprecation errors that run() caused.

Capture the thread's current loop with _get_current_event_loop()
before installing a new one, and restore it in the finally block. If
there was no previous loop, unset the loop with set_event_loop(None)
so that the thread is left without a loop instead of with the closed
one.

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
__init__() builds the parser and the lazy subcommand registration
inside it decides which subcommands to register by re-parsing
sys.argv. run() then parses a different argv, so if the caller passes
an argv that is deeper than the one in sys.argv, the required
subparsers have not been registered and the invocation fails with an
"unrecognized arguments" error. run_sub_commands() passes argv to
run(), so the mismatch is reachable from the public API.

Move the parser construction from __init__() into _build_parser() and
call it from run() when an argv is given, so that registration and
parsing are driven by the same command line. The top-level command
instances are created once in __init__() and reused when the parser
is rebuilt.

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
_run() receives the parsed arguments as its args parameter, but then
checks the private __args attribute for the func attribute and resolves
the command function through the args property. Both refer to the same
object today, so the mixing is harmless, but it obscures the data flow
and would silently diverge if a caller ever passed a namespace other
than the stored one.

Use the args parameter consistently in _run().

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
_add_arguments() adds the global options to self.__parser instead of
to the parser it receives. The two are the same object, because the
only caller passes self.__parser, so the change is not observable.

Use the parser parameter instead, so that the method honors its
argument the way Cmd.add_arguments() does, and so that the global
options can be shared with other parsers, e.g. the subcommand
parsers, without rewriting this method.

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
The subcommand registration in _build_parser() defines a SubCommand
helper class inside the add_cmds_to_parser() closure, so a fresh class
object is created on every call. It also stores command names and
aliases in a dictionary without checking for duplicates, so a colliding
name or alias is silently overwritten, and it relies on every subparser
level sharing the dest = 'command' attribute to descend one level per
re-parse, an invariant that is not documented anywhere.

Hoist the helper to a module-level _SubCommand NamedTuple, log a
warning when a subcommand name or alias collides with an earlier one at
the same level, and document the dest = 'command' invariant next to the
re-parse.

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
__run() only accepts a return value from _run() as the process exit
status if it is an int between 0 and 255, and silently drops any other
value. A command that returns, for instance, 300 therefore exits with
status 0, which presents a failure as a success to the caller without
any trace of the mistake.

Log an error when the returned exit status is out of range so that the
programming error is visible, while still exiting with 0 instead of
passing an invalid status to the shell.

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
The shell completion setup in __run() catches every exception and
silently ignores it. If argcomplete is missing or its initialization
fails, the completion is simply not available and there is no trace
of why, even when logging is turned up to debug level.

Log the reason at debug level: one message when the argcomplete
import fails and one with the exception for any other failure.

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
The os.getenv() calls in __init__() that read the log and backtrace
defaults mostly pass None as the explicit default value, but the one
for the show-backtrace environment variable relies on the implicit
default.

Pass None explicitly there as well, so that all the calls read the
same way.

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
lib.Cmd: Add single Cmd to add_subcommands()
All checks were successful
CI / Packaging - Kali Linux (pull_request) Successful in 4m32s
Details
CI / Packaging - OpenSUSE Tumbleweed (pull_request) Successful in 4m30s
Details
CI / Packaging test (pull_request) Successful in 0s
Details
CI / Packaging - Kali Linux (push) Successful in 4m18s
Details
CI / Packaging - OpenSUSE Tumbleweed (push) Successful in 3m59s
Details
CI / Packaging test (push) Successful in 0s
Details
75b6603a3f
 
add_subcommands() advertises Cmd and list[Cmd] in its signature, but
a single Cmd instance raises NotImplementedError, and since the list
branch handles every element through the same method, a list of Cmd
instances is broken as well. The only working forms are Types and
lists of Types.

Handle a single Cmd instance by reparenting it to the caller and
appending it to the children, tracking its class like the class-based
path does. Instances whose name is already taken by a child are
rejected, mirroring the duplicate-class handling, because argparse
cannot register two subparsers under the same name.

Assisted-by: unsloth/Qwen3.8-27B-GGUF:Q4_K_M with pi.dev v0.84.2
Signed-off-by: Jan Lindemann <jan@janware.com>
Jan Lindemann scheduled this pull request to auto merge when all checks succeed 2026-08-15 15:58:49 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
janware/jw-pkg!60
No description provided.