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 91.17647% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/lib/questions.rs	90.00%	6 Missing ⚠️

Files with missing lines	Coverage Δ
src/lib/config.rs	100.00% <100.00%> (ø)
src/lib/questions.rs	96.33% <90.00%> (-1.00%)	⬇️

🚀 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: ***@***.***>