Open
Conversation
Reviewer's GuideCentralizes default resolution of CLI arguments (especially output format, branch, path, exclude, pre-commit) and adds CI-aware auto-detection and propagation of output format across the CLI, core execution, serialization, and tests, while cleaning up MISSING sentinels and minor typing/logging improvements. Sequence diagram for CLI main with CI-aware output format resolutionsequenceDiagram
actor User
participant CLI as cli_main
participant Parser as argparse_ArgumentParser
participant Cfg as get_config
participant Resolver as _resolve_args
participant PCResolver as _resolve_pre_commit
participant CI as _detect_ci_provider
participant Core as core_check_or_pull
participant FmtFactory as get_formatter
participant Formatter as ResultFormatter
User->>CLI: invoke ruff_sync_cli
CLI->>Parser: parse_args()
Parser-->>CLI: args: CLIArguments
CLI->>Cfg: get_config(initial_to)
Cfg-->>CLI: config: Config
CLI->>Resolver: _resolve_args(args, config, initial_to)
Resolver->>Resolver: _resolve_upstream
Resolver->>Resolver: _resolve_to
Resolver->>Resolver: _resolve_exclude
Resolver->>Resolver: _resolve_branch
Resolver->>Resolver: _resolve_path
Resolver->>Resolver: _resolve_output_format
alt output_format from CLI
Resolver-->>Resolver: return CLI args.output_format
else output_format from config
Resolver-->>Resolver: parse config.output_format to OutputFormat
else auto detect in CI
Resolver->>CI: _detect_ci_provider()
CI-->>Resolver: provider or null
alt provider detected
Resolver-->>Resolver: map provider to OutputFormat
else no provider
Resolver-->>Resolver: use OutputFormat.TEXT
end
end
Resolver-->>CLI: ResolvedArgs (includes output_format)
CLI->>PCResolver: _resolve_pre_commit(args, config)
PCResolver-->>CLI: pre_commit bool
CLI->>CLI: build Arguments with resolved_upstream, branch, path, exclude, pre_commit, output_format
CLI->>Core: check or pull(Arguments)
Core->>FmtFactory: get_formatter(args.output_format)
FmtFactory-->>Core: Formatter
Core->>Formatter: note("Checking or syncing...")
Core->>Formatter: success or error messages
Core-->>CLI: exit_code
CLI-->>User: process exit_code
Class diagram for updated CLI and core argument resolutionclassDiagram
class OutputFormat {
<<enum>>
TEXT
JSON
GITHUB
GITLAB
SARIF
}
class CIProvider {
<<enum>>
GITHUB
GITLAB
SARIF
}
class Arguments {
<<NamedTuple>>
+str command
+tuple~URL~ upstream
+Path to
+Iterable~str~ exclude
+int verbose
+str branch
+str path
+bool semantic
+bool diff
+bool init
+bool pre_commit
+bool save
+OutputFormat output_format
}
class ResolvedArgs {
<<NamedTuple>>
+tuple~URL~ upstream
+Path to
+Iterable~str~ exclude
+str branch
+str path
+OutputFormat output_format
}
class CLIArguments {
<<protocol>>
+str command
+list~URL~ upstream
+str to
+str source
+list~str~ exclude
+int verbose
+str branch
+str path
+bool pre_commit
+OutputFormat output_format
+bool init
+bool save
+bool semantic
+bool diff
}
class Config {
<<TypedDict>>
+list~str~ exclude
+str branch
+str path
+bool pre_commit_version_sync
+str output_format
+str to
+str source
}
class ResultFormatter {
<<interface>>
+note(message str) void
+success(message str) void
+error(message str) void
}
class cli_module {
+get_config(source Path) Config
+_resolve_upstream(args CLIArguments, config Config) tuple~URL~
+_resolve_exclude(args CLIArguments, config Config) Iterable~str~
+_resolve_branch(args CLIArguments, config Config) str
+_resolve_path(args CLIArguments, config Config) str
+_resolve_output_format(args CLIArguments, config Config) OutputFormat
+_resolve_to(args CLIArguments, config Config, initial_to Path) Path
+_resolve_args(args CLIArguments, config Config, initial_to Path) ResolvedArgs
+_resolve_pre_commit(args CLIArguments, config Config) bool
+_detect_ci_provider() CIProvider
+main() int
}
class core_module {
+check(args Arguments) int
+pull(args Arguments) int
+serialize_ruff_sync_config(doc TOMLDocument, args Arguments) void
}
class constants_module {
+resolve_defaults(branch str, path str, exclude Iterable~str~, output_format OutputFormat) tuple
}
cli_module --> CLIArguments
cli_module --> ResolvedArgs
cli_module --> Arguments
cli_module --> Config
cli_module --> OutputFormat
cli_module --> CIProvider
core_module --> Arguments
core_module --> ResultFormatter
core_module --> OutputFormat
constants_module --> OutputFormat
constants_module --> Arguments
Arguments o--> OutputFormat
ResolvedArgs o--> OutputFormat
Config o--> OutputFormat
Arguments o--> Config
Arguments o--> ResolvedArgs
core_module ..> cli_module
core_module ..> constants_module
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/ruff_sync/cli.py" line_range="417-419" />
<code_context>
+ LOGGER.warning(f"Unknown output format in config: {fmt_str}")
+
+ # Auto-detection
+ provider = _detect_ci_provider()
+ if provider:
+ LOGGER.info(f"🤖 Auto-detected CI environment: {provider}")
+ return provider
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Auto-detected CI provider is returned as an OutputFormat, which is likely a type/enum mismatch.
Here `provider` is typed as `CIProvider | None`, but the function returns `OutputFormat | MissingType`. Returning `provider` directly means callers like `resolve_defaults(..., output_format)` and `get_formatter(output_format)` may receive a `CIProvider` where an `OutputFormat` is expected, which can break comparisons/formatter selection unless `CIProvider` is explicitly compatible with `OutputFormat`. Please either map CI providers to an `OutputFormat` here, or adjust the types so that relationship is explicit and enforced.
</issue_to_address>
### Comment 2
<location path="tests/test_ci_validation.py" line_range="88-97" />
<code_context>
+@pytest.mark.parametrize(
</code_context>
<issue_to_address>
**suggestion (testing):** Add test cases for explicit `--output-format text` and config `output-format = "text"` to cover non-auto-detected TEXT resolution
The current parametrized test covers CLI/config overriding CI, CI auto-detection, and the default `MISSING → TEXT` fallback, but it misses two explicit TEXT paths:
1. Explicit CLI flag: when `--output-format text` is set (with or without CI env vars), `_resolve_output_format` should return `OutputFormat.TEXT` directly, not `MISSING`. Please add a parametrized case like:
```python
(
{"GITHUB_ACTIONS": "true"},
{"output_format": OutputFormat.TEXT},
{},
OutputFormat.TEXT,
),
```
2. Config-only TEXT: when `output-format = "text"` is set in config without CI env vars, it should also resolve to `OutputFormat.TEXT`. For example:
```python
(
{},
{},
{"output-format": "text"},
OutputFormat.TEXT,
),
```
These will verify TEXT is correctly handled when chosen explicitly, not only via the default fallback.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+417
to
+419
| provider = _detect_ci_provider() | ||
| if provider: | ||
| LOGGER.info(f"🤖 Auto-detected CI environment: {provider}") |
There was a problem hiding this comment.
issue (bug_risk): Auto-detected CI provider is returned as an OutputFormat, which is likely a type/enum mismatch.
Here provider is typed as CIProvider | None, but the function returns OutputFormat | MissingType. Returning provider directly means callers like resolve_defaults(..., output_format) and get_formatter(output_format) may receive a CIProvider where an OutputFormat is expected, which can break comparisons/formatter selection unless CIProvider is explicitly compatible with OutputFormat. Please either map CI providers to an OutputFormat here, or adjust the types so that relationship is explicit and enforced.
Comment on lines
+88
to
+97
| @pytest.mark.parametrize( | ||
| ("env_vars", "cli_args", "config", "expected"), | ||
| [ | ||
| # CLI takes precedence | ||
| ( | ||
| {"GITHUB_ACTIONS": "true"}, | ||
| {"output_format": OutputFormat.JSON}, | ||
| {}, | ||
| OutputFormat.JSON, | ||
| ), |
There was a problem hiding this comment.
suggestion (testing): Add test cases for explicit --output-format text and config output-format = "text" to cover non-auto-detected TEXT resolution
The current parametrized test covers CLI/config overriding CI, CI auto-detection, and the default MISSING → TEXT fallback, but it misses two explicit TEXT paths:
- Explicit CLI flag: when
--output-format textis set (with or without CI env vars),_resolve_output_formatshould returnOutputFormat.TEXTdirectly, notMISSING. Please add a parametrized case like:
(
{"GITHUB_ACTIONS": "true"},
{"output_format": OutputFormat.TEXT},
{},
OutputFormat.TEXT,
),- Config-only TEXT: when
output-format = "text"is set in config without CI env vars, it should also resolve toOutputFormat.TEXT. For example:
(
{},
{},
{"output-format": "text"},
OutputFormat.TEXT,
),These will verify TEXT is correctly handled when chosen explicitly, not only via the default fallback.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #146 +/- ##
==========================================
+ Coverage 93.75% 93.94% +0.18%
==========================================
Files 6 6
Lines 1121 1156 +35
==========================================
+ Hits 1051 1086 +35
Misses 70 70 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Now that
cli.mainresolvesbranch,path, andexcludedefaults up front and_merge_multiple_upstreamsusesargs.branch/path/excludedirectly, consider simplifyingconstants.resolve_defaults(or removing its newoutput_formatparameter) if it is no longer used as a central source of truth. - In the
test_resolve_defaults_*tests you assertoutput_format == "text"; ifOutputFormatis an enum, it would be more robust to assert againstOutputFormat.TEXT(oroutput_format.value) to avoid relying on implicit enum-to-string equality. - When logging an unknown
output_formatin_resolve_output_format, consider including the validOutputFormatvalues in the warning message to make misconfigurations easier to diagnose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `cli.main` resolves `branch`, `path`, and `exclude` defaults up front and `_merge_multiple_upstreams` uses `args.branch/path/exclude` directly, consider simplifying `constants.resolve_defaults` (or removing its new `output_format` parameter) if it is no longer used as a central source of truth.
- In the `test_resolve_defaults_*` tests you assert `output_format == "text"`; if `OutputFormat` is an enum, it would be more robust to assert against `OutputFormat.TEXT` (or `output_format.value`) to avoid relying on implicit enum-to-string equality.
- When logging an unknown `output_format` in `_resolve_output_format`, consider including the valid `OutputFormat` values in the warning message to make misconfigurations easier to diagnose.
## Individual Comments
### Comment 1
<location path="tests/test_ci_validation.py" line_range="127-111" />
<code_context>
+ {},
+ OutputFormat.TEXT,
+ ),
+ # Unknown config format falls back to auto-detect/default (GitHub in this env)
+ (
+ {"GITHUB_ACTIONS": "true"},
+ {},
+ {"output_format": "invalid"},
+ OutputFormat.GITHUB,
+ ),
+ # Explicit CLI TEXT (with CI env)
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that an invalid config output_format emits a warning log
This parametrized case verifies the fallback to GitHub for an invalid `output_format`, but it doesn't assert the new logging behaviour in `_resolve_output_format`. Please use `caplog` here to assert that the expected warning (e.g. containing `Unknown output format in config`) is emitted, so changes to logging are caught by the test.
Suggested implementation:
```python
# Unknown config format falls back to auto-detect/default (GitHub in this env)
# and should emit a warning log about the unknown output_format
```
To fully implement the requested assertion using `caplog`, you will also need to update the parametrized test function that uses these cases:
1. **Inject `caplog` into the test function signature**
If the test currently looks roughly like:
```python
@pytest.mark.parametrize(
"env, cli_config, file_config, expected_output_format",
[...],
)
def test_resolve_output_format(env, cli_config, file_config, expected_output_format, monkeypatch):
...
```
change the signature to accept `caplog`:
```python
def test_resolve_output_format(env, cli_config, file_config, expected_output_format, monkeypatch, caplog):
```
2. **Add a conditional assertion for the invalid `output_format` case**
After calling `_resolve_output_format` (or whatever helper the test currently calls) and asserting `expected_output_format`, add a conditional block that checks for the invalid config case and asserts on the log:
```python
with caplog.at_level(logging.WARNING):
result = _resolve_output_format(env, cli_config, file_config)
assert result is expected_output_format
# Only the invalid config case should produce the warning
if file_config.get("output_format") == "invalid":
assert "Unknown output format in config" in caplog.text
else:
# Optionally assert that no warning was emitted in other cases
assert "Unknown output format in config" not in caplog.text
```
3. **Ensure logging is imported / configured**
At the top of `tests/test_ci_validation.py`, ensure:
```python
import logging
```
is present so `logging.WARNING` is available.
These changes will make the parametrized case that uses `{"output_format": "invalid"}` verify not only the fallback to `OutputFormat.GITHUB` but also that `_resolve_output_format` emits the expected warning log.
</issue_to_address>
### Comment 2
<location path="tests/test_constants.py" line_range="22-23" />
<code_context>
def test_resolve_defaults_passthrough():
"""Verify that non-MISSING values are passed through unchanged."""
- branch, path, exclude = resolve_defaults("develop", "src", ["rule1"])
+ branch, path, exclude, _ = resolve_defaults("develop", "src", ["rule1"])
assert branch == "develop"
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that explicitly verifies resolve_defaults preserves a non-MISSING output_format
Currently the tests only cover the default `TEXT` case and don’t assert the behaviour when a non-MISSING `output_format` is provided. Please add a test that calls something like `resolve_defaults(MISSING, MISSING, MISSING, OutputFormat.GITLAB)` and asserts that the returned `output_format` remains `OutputFormat.GITLAB`, so this contract is explicitly covered by tests.
Suggested implementation:
```python
def test_resolve_defaults_passthrough():
"""Verify that non-MISSING values are passed through unchanged."""
branch, path, exclude, _ = resolve_defaults("develop", "src", ["rule1"])
assert branch == "develop"
assert path == "src"
def test_resolve_defaults_output_format_passthrough():
"""Verify that a non-MISSING output_format is passed through unchanged."""
branch, path, exclude, output_format = resolve_defaults(
MISSING,
MISSING,
MISSING,
OutputFormat.GITLAB,
)
assert branch == DEFAULT_BRANCH
assert path is None # DEFAULT_PATH ("") is normalized to None
assert exclude == DEFAULT_EXCLUDE
assert output_format == OutputFormat.GITLAB
def test_resolve_defaults_mixed():
```
If `OutputFormat` is not yet imported in `tests/test_constants.py`, you will also need to add an import at the top of the file, for example:
- `from <your_module>.constants import OutputFormat` (or the appropriate module path where `OutputFormat` is defined).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| {"GITHUB_ACTIONS": "true"}, | ||
| {}, | ||
| {}, | ||
| OutputFormat.GITHUB, |
There was a problem hiding this comment.
suggestion (testing): Add an assertion that an invalid config output_format emits a warning log
This parametrized case verifies the fallback to GitHub for an invalid output_format, but it doesn't assert the new logging behaviour in _resolve_output_format. Please use caplog here to assert that the expected warning (e.g. containing Unknown output format in config) is emitted, so changes to logging are caught by the test.
Suggested implementation:
# Unknown config format falls back to auto-detect/default (GitHub in this env)
# and should emit a warning log about the unknown output_formatTo fully implement the requested assertion using caplog, you will also need to update the parametrized test function that uses these cases:
-
Inject
caploginto the test function signature
If the test currently looks roughly like:@pytest.mark.parametrize( "env, cli_config, file_config, expected_output_format", [...], ) def test_resolve_output_format(env, cli_config, file_config, expected_output_format, monkeypatch): ...
change the signature to accept
caplog:def test_resolve_output_format(env, cli_config, file_config, expected_output_format, monkeypatch, caplog):
-
Add a conditional assertion for the invalid
output_formatcase
After calling_resolve_output_format(or whatever helper the test currently calls) and assertingexpected_output_format, add a conditional block that checks for the invalid config case and asserts on the log:with caplog.at_level(logging.WARNING): result = _resolve_output_format(env, cli_config, file_config) assert result is expected_output_format # Only the invalid config case should produce the warning if file_config.get("output_format") == "invalid": assert "Unknown output format in config" in caplog.text else: # Optionally assert that no warning was emitted in other cases assert "Unknown output format in config" not in caplog.text
-
Ensure logging is imported / configured
At the top oftests/test_ci_validation.py, ensure:import logging
is present so
logging.WARNINGis available.
These changes will make the parametrized case that uses {"output_format": "invalid"} verify not only the fallback to OutputFormat.GITHUB but also that _resolve_output_format emits the expected warning log.
Comment on lines
-22
to
23
| branch, path, exclude = resolve_defaults("develop", "src", ["rule1"]) | ||
| branch, path, exclude, _ = resolve_defaults("develop", "src", ["rule1"]) | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Add a test that explicitly verifies resolve_defaults preserves a non-MISSING output_format
Currently the tests only cover the default TEXT case and don’t assert the behaviour when a non-MISSING output_format is provided. Please add a test that calls something like resolve_defaults(MISSING, MISSING, MISSING, OutputFormat.GITLAB) and asserts that the returned output_format remains OutputFormat.GITLAB, so this contract is explicitly covered by tests.
Suggested implementation:
def test_resolve_defaults_passthrough():
"""Verify that non-MISSING values are passed through unchanged."""
branch, path, exclude, _ = resolve_defaults("develop", "src", ["rule1"])
assert branch == "develop"
assert path == "src"
def test_resolve_defaults_output_format_passthrough():
"""Verify that a non-MISSING output_format is passed through unchanged."""
branch, path, exclude, output_format = resolve_defaults(
MISSING,
MISSING,
MISSING,
OutputFormat.GITLAB,
)
assert branch == DEFAULT_BRANCH
assert path is None # DEFAULT_PATH ("") is normalized to None
assert exclude == DEFAULT_EXCLUDE
assert output_format == OutputFormat.GITLAB
def test_resolve_defaults_mixed():If OutputFormat is not yet imported in tests/test_constants.py, you will also need to add an import at the top of the file, for example:
from <your_module>.constants import OutputFormat(or the appropriate module path whereOutputFormatis defined).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Auto-detect and propagate output format and other CLI defaults (branch, path, exclude, pre-commit) through the configuration, CLI, and core execution paths, simplifying argument handling and CI integration.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests: