Skip to content

Improve overall commands handling of envconfig#181

Merged
tomschr merged 7 commits intomainfrom
toms/command-fixes
Feb 17, 2026
Merged

Improve overall commands handling of envconfig#181
tomschr merged 7 commits intomainfrom
toms/command-fixes

Conversation

@tomschr
Copy link
Copy Markdown
Contributor

@tomschr tomschr commented Feb 16, 2026

Some of our docbuild commands checks for an empty .envconfig like this:

if context.envconfig is None:
        raise ValueError("No envconfig found in context.")

This is futile now and a leftover as it's all covered by Pydantic.

This PR changes:

  • Removed these futile if-clauses in src/docbuild/cli/cmd_c14n/* files.
  • Introduced a cast (env = cast(EnvConfig, context.envconfig)) to make editor not complain. => Change DocBuildContext.envconfig to be a EnvConfig object and not a dict.
  • Adjusted the test cases to make dummy EnvConfig objects and not dicts to simulate the behavior of the model.
  • Realized that ManagedGitRepo should allow str and Repo in __init__ to pass any of the two objects without taking care. This make it easier when called. Raise a TypeError when there is a different type.
  • Removed obsolete test_process_no_envconfig test function as there is no need to test this anymore when Pydantic already validates it. Failed configs are tested elsewhere.

* As Pydantic takes care of validation the envconfig,
  we don't have to check that anymore
* Remove any checking of if context.envconfig is None
* Adjust ManagedGitRepo to allow string or Repo as remote_url
  This makes it easier
* Use `cast(Envconfig, context.envconfig)` to avoid type errors
Most tests had a dict instead of a EnvConfig object. This fix
introduces a fake EnvConfig.
We have already Pydantic validation, no need to check.
@tomschr tomschr self-assigned this Feb 16, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 16, 2026

Coverage Report

For commit d3e2bca

Click to expand Coverage Report
  Name                                           Stmts   Miss Branch BrPart  Cover
  --------------------------------------------------------------------------------
+ src/docbuild/cli/cmd_check/process.py             58      0     22      1  98.8%
+ src/docbuild/utils/pidlock.py                     79      1     14      1  97.8%
+ src/docbuild/config/xml/stitch.py                 48      1     12      1  96.7%
+ src/docbuild/cli/cmd_validate/process.py         178      5     52      4  96.1%
+ src/docbuild/cli/callback.py                      35      0     10      2  95.6%
+ src/docbuild/cli/cmd_metadata/metaprocess.py     168     10     42      7  91.9%
- src/docbuild/cli/cmd_config/__init__.py            9      1      0      0  88.9%
- src/docbuild/cli/cmd_cli.py                       86     11      6      3  84.8%
- src/docbuild/cli/cmd_check/__init__.py            18      5      2      0  65.0%
- src/docbuild/cli/cmd_build/__init__.py            13      5      0      0  61.5%
- src/docbuild/cli/cmd_metadata/__init__.py         27     10      2      0  58.6%
- src/docbuild/cli/cmd_config/environment.py        11      6      2      0  38.5%
  --------------------------------------------------------------------------------
+ TOTAL                                           2734     55    624     19  97.6%
  
  47 files skipped due to complete coverage.

@tomschr tomschr added the kind:refactor Code cleanup without logic change. label Feb 16, 2026
@tomschr tomschr requested a review from sushant-suse February 16, 2026 17:14
@sushant-suse
Copy link
Copy Markdown
Collaborator

Some of our docbuild commands checks for an empty .envconfig like this:

if context.envconfig is None:
        raise ValueError("No envconfig found in context.")

This is futile now and a leftover as it's all covered by Pydantic.

This PR changes:

  • Removed these futile if-clauses in src/docbuild/cli/cmd_c14n/* files.
  • Introduced a cast (env = cast(EnvConfig, context.envconfig)) to make editor not complain.
  • Adjusted the test cases to make dummy EnvConfig objects and not dicts to simulate the behavior of the model.
  • Realized that ManagedGitRepo should allow str and Repo in __init__ to pass any of the two objects without taking care. This make it easier when called. Raise a TypeError when there is a different type.
  • Removed obsolete test_process_no_envconfig test function as there is no need to test this anymore when Pydantic already validates it. Failed configs are tested elsewhere.

Hi @tomschr, thanks a lot for the awesome work. Much appreciated!

This is a fantastic cleanup. By using our move to Pydantic, you've significantly reduced the 'noise' of defensive coding across the CLI commands.

The code is much leaner and follows better DevSecOps and Pythonic practices. LGTM, Approved.

One small request: could you please add a fragment file for example:

Streamlined CLI subcommands by removing redundant environment configuration checks and implementing strict typing with EnvConfig models. Updated ManagedGitRepo to support both string and Repo model initialization.

@tomschr
Copy link
Copy Markdown
Contributor Author

tomschr commented Feb 17, 2026

One small request: could you please add a fragment file

This is added in commit c1366ea

Declare envconfig as EnvConfig type in DocBuildContext
@tomschr
Copy link
Copy Markdown
Contributor Author

tomschr commented Feb 17, 2026

I also removed the cast(EnvConfig, ...) part as the envconfig attribute is now a EnvConfig object, not a dict anymore. This makes things better for our editors.

@sushant-suse
Copy link
Copy Markdown
Collaborator

I also removed the cast(EnvConfig, ...) part as the envconfig attribute is now a EnvConfig object, not a dict anymore. This makes things better for our editors.

Awesome. Please merge it.

@tomschr tomschr merged commit 3c41cee into main Feb 17, 2026
9 checks passed
@tomschr tomschr deleted the toms/command-fixes branch February 17, 2026 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:refactor Code cleanup without logic change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants