diff --git a/news/fix-root-key-logic.bugfix b/news/23.bugfix similarity index 100% rename from news/fix-root-key-logic.bugfix rename to news/23.bugfix diff --git a/news/24.internal b/news/24.internal new file mode 100644 index 0000000..f56c30f --- /dev/null +++ b/news/24.internal @@ -0,0 +1 @@ +Refactored `BaseRenderer.render` to reduce cyclomatic complexity and removed leftover debug log statements. @erral diff --git a/src/tui_forms/form/form.py b/src/tui_forms/form/form.py index e1b8f86..cfe549d 100644 --- a/src/tui_forms/form/form.py +++ b/src/tui_forms/form/form.py @@ -52,8 +52,7 @@ def is_active(self, question: BaseQuestion) -> bool: if self.root_key: answers = answers.get(self.root_key, {}) return all( - answers.get(cond["key"]) == cond["value"] - for cond in question.condition + answers.get(cond["key"]) == cond["value"] for cond in question.condition ) def start(self) -> None: diff --git a/src/tui_forms/form/question.py b/src/tui_forms/form/question.py index 6da7a74..467cc7e 100644 --- a/src/tui_forms/form/question.py +++ b/src/tui_forms/form/question.py @@ -63,9 +63,6 @@ def _render_variable( current_value = (answers.get(root_key, {}) if root_key else answers).get( key, _NOVALUE ) - import sys - if key == 'thumbor': - print(f"DEBUG: _render_variable({key}): root_key='{root_key}', found={current_value != _NOVALUE}, value={current_value}", file=sys.stderr) if current_value is _NOVALUE: value = template.render_variable(env, value, answers) else: diff --git a/src/tui_forms/parser.py b/src/tui_forms/parser.py index eae4410..66dcf37 100644 --- a/src/tui_forms/parser.py +++ b/src/tui_forms/parser.py @@ -273,8 +273,6 @@ def _build_subquestions( seen_keys.add(sub_key) for allof_item in prop_schema.get("allOf", []): - import sys - print(f"DEBUG: Processing allOf item: {allof_item}", file=sys.stderr) then = allof_item.get("then") if not then: continue diff --git a/src/tui_forms/renderer/base.py b/src/tui_forms/renderer/base.py index 5c5aab5..91b8731 100644 --- a/src/tui_forms/renderer/base.py +++ b/src/tui_forms/renderer/base.py @@ -65,44 +65,57 @@ def render( current_initial = initial_answers while True: self._form.start() - if current_initial: - # If root_key is set, and current_initial is not already nested - # under it, we should record them properly so they end up - # in the right place for is_active and other logic. - if self._form.root_key and self._form.root_key not in current_initial: - for k, v in current_initial.items(): - self._form.record(k, v) - else: - self._form.answers.update(current_initial) + self._apply_initial_answers(current_initial) self._ask_questions(self._form.questions) - # Remove stale answers for questions that became inactive - # (e.g. conditional questions whose gating answer changed - # after the user went back). - for question in self._form.iter_all(): - if not question.hidden and not self._form.is_active(question): - self._form.unrecord(question.key) - for question in self._form.iter_all(): - if question.hidden and self._form.is_active(question): - self._form.record( - question.key, - question.default_value( - self._env, self._form.answers, self._form.root_key - ), - ) + self._clean_up_inactive_answers() + self._record_hidden_answers() answers = dict(self._form.answers) if not confirm or self.render_summary(self._form.user_answers): return answers - # Exclude computed (hidden) fields so they are re-evaluated - # from scratch on the next pass using the updated answers. - computed_keys = {q.key for q in self._form.iter_all() if q.hidden} - if self._form.root_key and self._form.root_key in answers: - inner = answers[self._form.root_key] - filtered = {k: v for k, v in inner.items() if k not in computed_keys} - current_initial = {self._form.root_key: filtered} - else: - current_initial = { - k: v for k, v in answers.items() if k not in computed_keys - } + current_initial = self._get_initial_for_retry(answers) + + def _apply_initial_answers(self, initial: dict[str, Any] | None) -> None: + """Apply initial answers to the form.""" + if not initial: + return + # If root_key is set, and initial is not already nested + # under it, we should record them properly so they end up + # in the right place for is_active and other logic. + if self._form.root_key and self._form.root_key not in initial: + for k, v in initial.items(): + self._form.record(k, v) + else: + self._form.answers.update(initial) + + def _clean_up_inactive_answers(self) -> None: + """Remove stale answers for questions that became inactive.""" + # (e.g. conditional questions whose gating answer changed + # after the user went back). + for question in self._form.iter_all(): + if not question.hidden and not self._form.is_active(question): + self._form.unrecord(question.key) + + def _record_hidden_answers(self) -> None: + """Compute and record values for active hidden questions.""" + for question in self._form.iter_all(): + if question.hidden and self._form.is_active(question): + self._form.record( + question.key, + question.default_value( + self._env, self._form.answers, self._form.root_key + ), + ) + + def _get_initial_for_retry(self, answers: dict[str, Any]) -> dict[str, Any]: + """Prepare initial answers for a form restart after review.""" + # Exclude computed (hidden) fields so they are re-evaluated + # from scratch on the next pass using the updated answers. + computed_keys = {q.key for q in self._form.iter_all() if q.hidden} + if self._form.root_key and self._form.root_key in answers: + inner = answers[self._form.root_key] + filtered = {k: v for k, v in inner.items() if k not in computed_keys} + return {self._form.root_key: filtered} + return {k: v for k, v in answers.items() if k not in computed_keys} def render_summary(self, user_answers: dict[str, Any]) -> bool: """Display a summary of user-provided answers and ask for confirmation. @@ -237,7 +250,6 @@ def _ask_questions(self, questions: list[form.BaseQuestion]) -> None: history.pop() continue self._form.record(next_q.key, answer, user_provided=self._user_provided) - # print(f"DEBUG: Recorded {next_q.key}={answer} (root_key={self._form.root_key})") history.append(next_q.key) def _dispatch(self, question: form.BaseQuestion) -> Any: diff --git a/tests/form/test_form_root_key.py b/tests/form/test_form_root_key.py index 40f4199..fd62483 100644 --- a/tests/form/test_form_root_key.py +++ b/tests/form/test_form_root_key.py @@ -1,6 +1,6 @@ from tui_forms.form.form import Form from tui_forms.form.question import Question -import pytest + def test_is_active_with_root_key(): """Verify that is_active correctly finds answers when a root_key is used.""" @@ -10,31 +10,33 @@ def test_is_active_with_root_key(): title="Thumbor", description="", default=False, - condition=[{"key": "postgres", "value": True}] + condition=[{"key": "postgres", "value": True}], ) - + # Without root_key frm1 = Form(title="T", description="", questions=[q_cond]) frm1.record("postgres", True) assert frm1.is_active(q_cond) is True - + # With root_key (e.g. cookiecutter) frm2 = Form(title="T", description="", questions=[q_cond], root_key="cookiecutter") frm2.record("postgres", True) # This is expected to FAIL before the fix assert frm2.is_active(q_cond) is True + def test_user_answers_with_root_key(): """Verify that user_answers correctly retrieves answers when a root_key is used.""" q = Question(key="a", type="string", title="A", description="", default="") frm = Form(title="T", description="", questions=[q], root_key="cookiecutter") - + frm.record("a", "val", user_provided=True) - + # This should return {'a': 'val'} # Before fix it might raise KeyError or return wrong data depending on implementation assert frm.user_answers == {"a": "val"} + def test_user_answers_empty_with_root_key(): """Verify that user_answers doesn't crash when no answers recorded yet with root_key.""" frm = Form(title="T", description="", questions=[], root_key="cookiecutter")