Scopes

[philocalyst]

No description provided.

[cococonscious]

Besides the conflicts, make sure to consider my comment in the previous PR: #161 (comment) and imo it should also be possible to configure to force the use of the configured scopes or not (and with that if an empty scope is allowed or not)

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 26 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/lib/questions.rs	84.44%	14 Missing ⚠️
src/lib/scope.rs	91.89%	9 Missing ⚠️
src/lib/config.rs	90.90%	3 Missing ⚠️

Files with missing lines	Coverage Δ
src/lib/config.rs	95.52% <90.90%> (-4.48%)	⬇️
src/lib/scope.rs	91.89% <91.89%> (ø)
src/lib/questions.rs	92.09% <84.44%> (-5.25%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[cococonscious]

Looks good besides the review comment, also, I have two other points to consider:

• The force_scope config key naming is a bit misleading, I think a more accurate name would be force_config_scopes
• We should show the configured scope description in the selection prompt, not only because it's otherwise obsolete but also to differentiate from the detected scopes

[philocalyst]

I'll address #2, but #1 I wrote because it's namespaced under Config. So Config.force_config_scopes feels redundant. You'll see updates later today. ▰▰▰▰▰ Miles Wirth 🙃

…

From: Finley Thomalla ***@***.***> Sent: 11 March 2026 01:07 To: cococonscious/koji ***@***.***> Cc: Miles Wirht ***@***.***>, Author ***@***.***> Subject: Re: [cococonscious/koji] Scopes (PR #179) @cococonscious requested changes on this pull request. Looks good besides the review comment, also, I have two other points to consider: - The `force_scope` config key naming is a bit misleading, I think a more accurate name would be `force_config_scopes` - We should show the configured scope description in the selection prompt, not only because it's otherwise obsolete but also to differentiate from the detected scopes > @@ -148,6 +163,21 @@ impl Autocomplete for ScopeAutocompleter { } fn prompt_scope(config: &Config) -> Result<Option<String>> { + if config.force_scope && !config.commit_scopes.is_empty() { + let scope_values: Vec<String> = config.commit_scopes.keys().cloned().collect(); + + let prompt = Select::new("What's the scope of this change?", scope_values) + .with_render_config(get_render_config()); + + let result = if config.allow_empty_scope { + prompt.prompt_skippable()? + } else { + Some(prompt.prompt()?) Pressing \<Esc> cancels the prompt with a generic error message, please find a way to improve it (for example "Error: Scope cannot be empty" -- Reply to this email directly or view it on GitHub: #179 (review) You are receiving this because you authored the thread. Message ID: ***@***.***>

[cococonscious]

I understand your point about the scoping of the config property, and it does make sense, but it's a bit confusing because "force scope" could be misunderstood as "you must specify a scope" (which the other variable is for so it is understandable, yes, but it does require an extra thinking step).

Thanks for the changes. I did find a bug though, when setting the following: force_scope = false and allow_empty_scope = false, hitting <Esc> will just continue instead of throwing like when force_scope = true.

Also, I find it hard to justify adding another error handling dependency, could we not either replace everything with thiserror or implement your changes in anyhow?

Lastly, the descriptions of the commit scopes defined in the config are still not visible.

Thanks,
Finley

[philocalyst]

The error handling thing: I think if we want to move to stronger error handling, the answer is 100% thiserror!! It's meant for structured error cases. The other thing we could consider is [Ariadne](https://lib.rs/crates/ariadne) or [Miette](https://lib.rs/crates/miette), which would place us in the top 1% for these diagnostics, but would require a larger refactor. I could start a separate PR that adds that to merge before this one so consistency stays. I also was thinking that globbing would pair nicely with this scope configuration, as a lot of scopes are bound purely to specific sets of files from within a codebase. I have a scope for my flake.nix for example. That way users could manage LOTS of scopes without it polluting the main dialog, which would be reserved for the more universal options. ▰▰▰▰▰ Miles Wirth 🙃

…

From: Finley Thomalla ***@***.***> Sent: 13 March 2026 03:42 To: cococonscious/koji ***@***.***> Cc: Miles Wirht ***@***.***>, Author ***@***.***> Subject: Re: [cococonscious/koji] Scopes (PR #179) cococonscious left a comment (cococonscious/koji#179) I understand your point about the scoping of the config property, and it does make sense, but it's a bit confusing because "force scope" could be misunderstood as "you must specify a scope" (which the other variable is for so it is understandable, yes, but it does require an extra thinking step). Thanks for the changes. I did find a bug though, when setting the following: `force_scope = false` and `allow_empty_scope = false`, hitting \<Esc> will just continue instead of throwing like when `force_scope = true`. Also, I find it hard to justify adding another error handling dependency, could we not either replace everything with `thiserror` or implement your changes in `anyhow`? Lastly, the descriptions of the commit scopes defined in the config are still not visible. Thanks, Finley -- Reply to this email directly or view it on GitHub: #179 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[cococonscious]

I think if we want to move to stronger error handling, the answer is 100% thiserror!! ... I could start a separate PR that adds that to merge before this one so consistency stays.

Yes, please! That would be great.

The other thing we could consider is Ariadne or Miette, ...

While they look great, I think they're overkill for the simple errors that are common in koji, unless you disagree...

I also was thinking that globbing would pair nicely with this scope configuration, ...

Sorry, I'm a bit lost on what exactly you mean by this, could you give me a concrete example?

[philocalyst]

Okay, I'll look into the error handling sometime soon! They are basically overkill, just thinking if the config becomes a bigger interface it would help, and we can fallback to the default case elsewhere? But definitely to think about LATER. Yes, something like flakes = "/flake.nix" core = "/crates/core/**/*.rs" build = ["/build.rs", "/justfile"] So if changes impact a match, the scope is pre-assigned. Which assigns these things. On second thought, a Regex or FancyRegex implementation could make more sense, as I see the deliniation getting advanced, and arrays feel like a clunky answer. ▰▰▰▰▰ Miles Wirth 🙃

…

From: Finley Thomalla ***@***.***> Sent: 17 March 2026 02:10 To: cococonscious/koji ***@***.***> Cc: Miles Wirht ***@***.***>, Author ***@***.***> Subject: Re: [cococonscious/koji] Scopes (PR #179) cococonscious left a comment (cococonscious/koji#179) > I think if we want to move to stronger error handling, the answer is 100% thiserror!! ... I could start a separate PR that adds that to merge before this one so consistency stays. Yes, please! That would be great. > The other thing we could consider is [Ariadne](https://lib.rs/crates/ariadne) or [Miette](https://lib.rs/crates/miette), ... While they look great, I think they're overkill for the simple errors that are common in koji, unless you disagree... > I also was thinking that globbing would pair nicely with this scope configuration, ... Sorry, I'm a bit lost on what exactly you mean by this, could you give me a concrete example? -- Reply to this email directly or view it on GitHub: #179 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[philocalyst]

Following up @cococonscious

[cococonscious]

Sorry @philocalyst for the delayed answer.

I think that's a great idea, especially if we use a regex implementation instead of arrays.

How would we handle precedence? In your example, if both "flake.nix" and "build.rs" are modified, would "flakes" or "build" be the chosen scope (if any)?

[philocalyst]

The thing about working on a tool for commits is that you develop standards just by making ergonomic decisions like this. I personally lean towards the atomic commit model, so my answer is "neither" as I believe any situation where you need to make that cross cutting change should be grouped into a PR.

If that's something you agree with, that can be the stance, but long-term we may need to adopt a particular committing philosphy as to not grow too complex.

[philocalyst]

Regex is also a great idea, in that we could use the implementation of ast-grep to both match on filenames and on code structure. Then you could do things like "if this modified function holds the test annotation, mark its scope as test".

Which is very exciting and a fair bit more technical, but I believe personally do-able. I volunteer it really because it differentiates Koji and uses ast-grep in a creative way (I find it difficult to use practically otherwise, despite how capable it is)

[philocalyst]

We could use ast_greps YAML input (Appropriated to TOML of course), which I found when researching -- it makes incorporating this feel very "right" with the current config structure :)))

[cococonscious]

I agree with you on the atomic commits, although I suspect that in reality most people using koji will not fully adopt that philosophy, so your (great) idea about using ast-grep might not work as expected for everyone. I think it would be an awesome addition, but it should be gated behind a feature flag for now, until we get some experience and user reports about how they use and like it. The regex config approach can be in the default feature-set though. If you're willing to, please do implement both 🥇
I'll try to review it faster than I've been replying in this thread, promise :)

[philocalyst]

Feeling a lot better about this! @cococonscious