Skip to content

Interactive mode: an answer applies to one occurrence, not the whole run - #3990

Open
Eljees wants to merge 4 commits into
codespell-project:mainfrom
Eljees:fix/62-interactive-answer-scope
Open

Interactive mode: an answer applies to one occurrence, not the whole run#3990
Eljees wants to merge 4 commits into
codespell-project:mainfrom
Eljees:fix/62-interactive-answer-scope

Conversation

@Eljees

Copy link
Copy Markdown
Contributor

Fixes#62.

The root cause is that ask_for_word_fix answers a question about one occurrence by mutating the shared Misspelling object, so a single answer leaks into every later occurrence of the word in the run. Observed on current master (all transcripts scripted, answers fed on stdin):

  • -w -i 1, the same word in two files, answers n, y: the second file never asks — the rejection turned the word off globally, and the queued y is never consumed.
  • -w -i 3, reject a word, then meet it in the next file: the prompt degrades from (Y/n) to Choose an option (blank for none): 0) ... — the "handled as a list of available fixes" behaviour from the issue, still alive.
  • -w -i 2, a word with a single candidate, answer blank ("blank for none"): the file is changed anyway — FIXED: c.txt.

Three changes:

  1. ask_for_word_fix no longer mutates misspelling; it returns the answer for this occurrence.
  2. The caller remembers answers per word per file (asked_for becomes a dict and moves out of the per-line scope, where its reset meant "asked once per file" only held because of the mutation). Accepting and rejecting now behave symmetrically: each word is asked once per file, and the answer covers the rest of that file.
  3. The option-list branch now triggers only for words with more than one candidate, which is what --help says level 2 is for ("ask user to choose one fix when more than one is available"). Single-candidate words at -i 2 are written without a prompt, as documented; the blank-accepts-anyway path is gone.

These are the first tests for interactive mode in the suite; they feed answers through stdin. On the unpatched tree the three bug tests fail exactly on the behaviours above, the per-file control passes; with the fix all four pass. Full test_basic.py: 86 passed, 3 failed — the same three failures (chardet import) as on a clean tree. ruff check, ruff format --check clean; mypy reports the same two pre-existing errors as on a clean tree.

AI-assisted (LLM used for drafting); the runs above are mine.

@Eljees

Copy link
Copy Markdown
ContributorAuthor

Ping — this has been open since 7 August with no review yet.

ask_for_word_fix answers a question about one occurrence by mutating the shared Misspelling object, so a single answer leaks into every later occurrence of that word in the run. Scripted transcripts on current master, answers fed on stdin:

  • -w -i 1, same word in two files, answers n then y — the second file is never asked; the rejection turned the word off globally and the queued y is never consumed.
  • -w -i 3, reject a word then meet it in the next file — the prompt degrades from (Y/n) to Choose an option (blank for none), which is the behaviour Interactive lvl 1 has undesired behavior #62 describes and it is still alive.
  • -w -i 2, a word with one candidate, blank answer — the file is changed anyway.

Only pre-commit.ci has run on it. The four GitHub Actions check suites for 811472a are at action_required with zero runs, so the test suite needs a maintainer to approve the workflow.

Happy to close it if you would rather solve #62 differently.

@larsoner

Copy link
Copy Markdown
Member

@Eljees sorry for the slow response... to help me get to it I had Claude Opus 5 take a look and expand it a bit by having "A" (all) and "S" (skip) options in addition to just "Y" / "N". Can you look and see if you agree it's better?

@Eljees

Copy link
Copy Markdown
ContributorAuthor

Yes - it is better, and the a/s split is the right call. Making a plain y/n cover exactly one match is what #62 actually asks for; my version cached per word per file, which is the same leak as master's with a smaller radius.

Three of your changes fix things I had wrong:

  • lineno=i + 1 -> line_number + 1. Inside a fragment i is the offset within the fragment, so the prompt showed the wrong line for anything after an ignored region.
  • Moving asked_for up to parse_file. Mine was per fragment, so --ignore-multiline-regex re-asked after the skipped block. test_interactive_whole_file_answer_spans_fragments pins it.
  • Returning the dictionary form and casing per match. That is necessary once an answer is per match, and it was not when the answer was per file.

The EOF guard is worth having on its own - an unanswered prompt used to read as y.

I re-ran the transcripts from my earlier note against eb81dfe4:

caseresult
-w -i 3, n then y, same word in two files2 prompts, first file left alone, second fixed; the prompt no longer degrades to Choose an option
-w -i 2, multi-candidate word, blank answerasked for both matches, file unchanged
-w -i 1, answers run out mid-runNo answer once per file, nothing changed after that, exit 65

One gap. a/s exist only in the interactivity & 1 branch. A word with more than one candidate goes to Choose an option, where a is answered with "Not a valid option", so under -i 2 or -i 3 you answer once per match with no way out - three occurrences of aache under -i 3 is three prompts. Before this change the answer stuck, via the mutation, so this is the one place where the new semantics cost the user more.

Either accepting a/s alongside the numbers there, or treating a blank answer as "leave the rest of this file alone", would close it. Happy to add whichever you prefer, or to leave that branch as it is if you would rather keep it simple.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interactive lvl 1 has undesired behavior

2 participants

@Eljees@larsoner