Conversation
89f6898 to
5aa0956
Compare
5aa0956 to
7db38a1
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9413 +/- ##
==========================================
- Coverage 76.47% 76.45% -0.03%
==========================================
Files 85 86 +1
Lines 14803 14905 +102
Branches 2213 2248 +35
==========================================
+ Hits 11321 11396 +75
- Misses 2803 2820 +17
- Partials 679 689 +10 ☔ View full report in Codecov by Sentry. |
14adfd2 to
c6452f7
Compare
|
@mauvilsa can you have a look, please? maybe look at the individual commits. is that had to import some internal stuff to implement guess this can soon be merged - it does not use most features of jsonargparse, but that should be in future PRs. |
c6452f7 to
5b9b450
Compare
|
Note: I am currently trying to get rid of these _maincommand, _midcommand, _subcommand suffixes. Guess they are not needed because jsonargparse supports nested namespaces to separate the different command levels. Also, the suffixes showed up in generated configs, making them ugly... Update: done! |
mauvilsa
left a comment
There was a problem hiding this comment.
I haven't tried to run the code yet. But might do that in case I see weird behaviors.
src/borg/archiver/debug_cmd.py
Outdated
| "info", | ||
| parents=[common_parser], | ||
| subparser = ArgumentParser( | ||
| parents=[mid_common_parser], |
There was a problem hiding this comment.
I think there aren't tests in jsonargparse related to parent parsers. Could be that it works just fine. But I should take note of this to have a closer look.
There was a problem hiding this comment.
it seems to work, at least neither our automated tests nor me practically trying it noticed an issue yet.
| from jsonargparse._actions import _ActionSubCommands # noqa: F401 | ||
| from jsonargparse._completions import prepare_actions_context, shtab_prepare_actions # noqa: F401 | ||
| from jsonargparse._completions import bash_compgen_typehint # noqa: F401 |
There was a problem hiding this comment.
I will look at this in more detail. Eventually there shouldn't be private imports. But I need to see if there is already an alternative, or a modification in jsonargparse is required.
d39fa19 to
2675b31
Compare
6665f8f to
21f7548
Compare
Previously, ArgparsePatternAction and ArgparsePatternFileAction appended recursion roots directly to args.paths. This mixed CLI positional paths with paths derived from patterns (e.g., using the `R` root path command in a pattern file), complicating downstream argument parsing and future jsonargparse integration. This commit introduces `args.pattern_roots` as a dedicated list for these accumulated root paths: - All argparse definition sites now initialize `pattern_roots=[]` alongside `paths=[]` - ArgparsePatternAction and ArgparsePatternFileAction write directly to `args.pattern_roots` - The build_matcher utility accepts both `include_paths` and `pattern_roots` and concatenates them internally - `create_cmd` iterations explicitly concatenate both lists before processing This ensures `args.paths` strictly reflects exactly what the user provided positionally, paving the way for a clean jsonargparse implementation without regressions in pattern behavior.
Guess it was triggered due to naming the variable "token", maybe "sort_key" is less problematic.
other validators / specs are also there and it is easier to maintain as Python code. the compress module is Cython code.
…approach The old code worked around argparse's flat namespace by appending _maincommand / _midcommand / _subcommand suffixes to every common option's dest (e.g. log_level_subcommand), then resolving them with CommonOptions.resolve() after parsing. This polluted config key names and env var names (BORG_LOG_LEVEL_SUBCOMMAND instead of BORG_LOG_LEVEL). jsonargparse nests subcommand arguments automatically, so the workaround is no longer needed. Each parser level now registers common options with their clean dest name. flatten_namespace() is updated to a two-pass depth-first walk so the most-specific (innermost) value wins naturally: borg --info create --debug → log_level = "debug" (subcommand wins) borg --info create → log_level = "info" (top-level fills gap) For append-action options (--debug-topic) values from all levels are merged (outer + inner) to preserve the accumulation behaviour.
Borg's ArgumentParser (in borg.helpers.argparsing) now subclasses jsonargparse's ArgumentParser and pre-sets two defaults that every borg parser uses: formatter_class = RawDescriptionHelpFormatter add_help = False
21f7548 to
e4e484f
Compare
|
(rebased on current master, added auto env var support) |
|
Hmm, I can't get "store_true" stuff working correctly: With that and without the value being set in the default config, overriding via If I set the value in the default config, it works, but I can not overwrite it with the env var. A hack with ActionYesNo also didn't work, because it does not like short options like Another weird hack: omni-us/jsonargparse#497 - is that the intended way? |
This I would consider this a bug. I will fix it. |
| parse_file_size, | ||
| ) | ||
| from ..helpers.argparsing import ArgumentParser | ||
| from ..helpers.argparsing import _ActionSubCommands |
There was a problem hiding this comment.
Some time ago I noticed that add_subcommand() does not show up in the API reference. I will make this class public to fix this and so that it can be used like here.
| prog = "borg" | ||
| preambles = [] | ||
| if args.shell == "bash": | ||
| preambles.append(bash_compgen_typehint.strip().replace("%s", prog)) | ||
| preambles.append(bash_preamble) | ||
| elif args.shell == "zsh": | ||
| preambles.append(zsh_preamble) | ||
|
|
||
| with prepare_actions_context(args.shell, prog, preambles): | ||
| shtab_prepare_actions(parser) | ||
|
|
||
| script = shtab.complete(parser, shell=args.shell, preamble="\n".join(preambles)) # nosec B604 |
There was a problem hiding this comment.
For this I am thinking that a method is needed to programmatically get a completion script. The code would change as follows:
| prog = "borg" | |
| preambles = [] | |
| if args.shell == "bash": | |
| preambles.append(bash_compgen_typehint.strip().replace("%s", prog)) | |
| preambles.append(bash_preamble) | |
| elif args.shell == "zsh": | |
| preambles.append(zsh_preamble) | |
| with prepare_actions_context(args.shell, prog, preambles): | |
| shtab_prepare_actions(parser) | |
| script = shtab.complete(parser, shell=args.shell, preamble="\n".join(preambles)) # nosec B604 | |
| preambles = [] | |
| if args.shell == "bash": | |
| preambles.append(bash_preamble) | |
| elif args.shell == "zsh": | |
| preambles.append(zsh_preamble) | |
| script = parser.get_completions_script(f"shtab-{args.shell}", preambles=preambles) |
The method would not be shtab specific so that it can be used in potential future extensions, e.g. omni-us/jsonargparse#618 (comment)
…oolean flags Root causes ----------- Two independent bugs conspired to make `BORG_CREATE__STATS=true` a no-op when `--stats` was not also given on the CLI: 1. **`ActionYes` default was `False` instead of `None`.** jsonargparse's config-merging pipeline calls `merge_config()` to layer CLI values on top of env-var values. Because every absent `--stats` flag produced a hard `False` default, that `False` was written into the merged namespace and silently overwrote the `True` that had been read from the environment variable. 2. **`flatten_namespace()` converted every `None` to `False` unconditionally.** After the namespace was flattened, any field that was still `None` (meaning "not set anywhere") was turned into `False`. This was correct for boolean flags, but the blanket conversion also masked the symptom of bug #1 and made it impossible to distinguish "flag absent" from "flag explicitly set to False". Fixes ----- * **`ActionYes` default changed from `False` to `None`** so that an absent CLI flag contributes nothing to the merge rather than a hard `False`. `_boolean_type()` is updated to pass `None` through unchanged. * **`ArgumentParser.merge_config()` override** added with two rules: - Skip any `None` value from `cfg_from` so it never overwrites a real value (e.g. `True` from an env var) in `cfg_to`. - For `ActionYes` fields specifically, a `False` in `cfg_from` must not overwrite a `True` already present in `cfg_to` (env-var `True` wins). * **`flatten_namespace()` now accepts `action_yes_dests`** and converts `None → False` only for those fields, leaving non-boolean `None` values untouched. * **`parse_args()`** collects all `ActionYes` dests from the parser and every subparser and passes them to `flatten_namespace()`. * **Regression test** `test_env_var_bool_flag` added: sets `BORG_CREATE__STATS=true`, parses `["create", "test", "/some/path"]` without `--stats`, and asserts `args.stats is True`.
The previous commit added a merge_config override that used an action_yes_dests set-comprehension to decide which fields to protect. This is unnecessary — the two generic rules are sufficient: 1. Never let a None value from cfg_from overwrite anything in cfg_to. (None means "not set" — it comes from ActionYes's default=None when the flag was absent from the CLI.) 2. Never let a False from cfg_from overwrite a True in cfg_to. (This protects an env-var True from being clobbered by a baked-in False default that jsonargparse's get_defaults/handle_subcommands injects into the top-level namespace before the subparser reads its own env vars.) These rules are safe for all field types, not just ActionYes booleans, so the action_yes_dests lookup in merge_config is removed. The action_yes_dests property and its use in flatten_namespace (to convert remaining None → False only for boolean flags) are kept — that part is still needed.
Description
Adopt jsonargparse, fixes #6551.
Checklist
master(or maintenance branch if only applicable there)toxor the relevant test subset)