Fix Markdown/bulk export review follow-ups from PR #19#20
Open
bigbruno wants to merge 1 commit into
Open
Conversation
Apply the ten code-review findings on the freshly-merged Markdown +
bulk export workflow. No new features; correctness and UI/UX only.
Converter (tsv_odf_converter.py):
- _MD_LINE_START_RE: require whitespace (or end of string) after the
digits-dot pattern so paragraphs starting with a decimal like
"1.5 million" or "2025.06 release" are no longer mangled to
"\1.5 million".
- _emit_kv: emit blank lines before and after so adjacent KV elements
no longer collapse into one CommonMark paragraph and the bolded key
stays visually distinct.
- _emit_paragraph: when DocElement.raw_lines preserved per-line breaks
(multi-line addresses, poetry), emit each line with a CommonMark
hard break (two trailing spaces) instead of collapsing into a single
run-on line.
Markdown export UI (conclusion_export_mixin.py):
- _export_markdown_file: write to "<path>.tmp" then os.replace so a
failed/cancelled conversion no longer destroys a pre-existing file
the user picked to overwrite via FileDialog.
- _on_md_save_response: guard against Gio.File.get_path() == None
(remote/MTP/GVfs locations) and surface a clear toast instead of an
AttributeError swallowed as a generic "Export failed".
- _on_md_export_clicked → _on_md_save_response → _export_markdown_file
→ _on_md_export_finished: pass include_front_matter and open_after
through the closure chain instead of self attributes, so overlapping
per-row exports can no longer clobber each other's settings.
- _on_folder_chosen (bulk): show _EXPORT_FAILED_MSG on non-Dismiss
errors so the user gets feedback instead of a silently-vanishing
dialog; also guards remote folders with the same toast as the
single-file path.
- _run_bulk_export / _bulk_convert_one: snapshot md_include_front_matter
and odf_include_images once at batch start and pass them via an
options dict, so a mid-batch toggle from another dialog can no longer
produce a non-uniform batch.
- _bulk_convert_one (md branch): write to "<path>.tmp" and os.replace,
matching the single-file atomic-write pattern.
- _bulk_export_worker: when fmt is not in _BULK_EXTENSIONS, close the
dialog and toast a real export-failed message instead of falling
back to a misleading "Saved 0 files".
- _build_progress_dialog: cancel handler now disables the button,
swaps its label and AT-SPI accessible name to "Cancelling…", and
rewrites the subtitle to "Finishing current step…". The progress
callback short-circuits after cancel so the message doesn't get
overwritten by late per-file updates. Gives the user immediate
feedback while parse_tsv_pages finishes its current step on a long
PDF.
Settings persistence (services/settings.py):
- Add _load_md_settings / _save_md_settings handling the
md_export.include_front_matter and md_export.open_after_export
config keys; hooked into load_settings / _save_all_settings.
- _update_md_setting (UI) now calls settings._save_md_settings()
before config.save() so the Markdown toggles actually persist
across restarts, matching the ODF flow.
Tests (test_markdown_export.py):
- TestEscapeMd.test_decimal_at_line_start_not_escaped: pins the new
decimal-aware behavior; "1." alone still escapes via the end-of-
string boundary.
- TestCreateMarkdown.test_kv_elements_separated_by_blank_line: pins
the new KV separator rule.
- TestCreateMarkdown.test_paragraph_preserves_raw_lines: pins the new
hard-break handling for multi-line OCR paragraphs.
- TestSaveMdSettings.{test_save_md_settings_writes_both_keys,
test_load_md_settings_reads_both_keys,
test_save_md_settings_defaults_when_unset}: cover the new persistence
path end-to-end with a minimal config double.
364 passing tests, ruff clean.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 20 |
| Duplication | 2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
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
Address the ten findings from the post-merge code review of PR #19. Pure correctness + UI/UX polish — no new features.
Converter (
utils/tsv_odf_converter.py)_MD_LINE_START_REnow requires whitespace/EOL after\d+\.so1.5 million/2025.06 releaseno longer escape to\1.5 million._emit_kvemits blank lines before and after so adjacent KV entries stay distinct CommonMark paragraphs._emit_paragraphhonoursDocElement.raw_linesvia hard breaks (two trailing spaces) for multi-line addresses, poetry, etc.Markdown export UI (
ui/conclusion_export_mixin.py)*.tmp+os.replace) so a failed/cancelled conversion no longer destroys a pre-existing target file.Gio.File.get_path() is None) for both the single-file save dialog and the bulk folder picker — clear toast instead of swallowedAttributeError.include_front_matter,open_after) instead ofselfattributes, so overlapping dialogs can't clobber each other._EXPORT_FAILED_MSG._run_bulk_exportsnapshotsmd_include_front_matter/odf_include_imagesat batch start; a mid-batch toggle can no longer produce a non-uniform batch.fmtin_BULK_EXTENSIONScloses the dialog and toasts a real failure instead of "Saved 0 files"._build_progress_dialognow disables itself, swaps label + AT-SPI accessible name toCancelling…, and rewrites subtitle toFinishing current step…. Progress callback short-circuits after cancel so late updates can't overwrite the message — gives immediate feedback whileparse_tsv_pagesfinishes.Settings persistence (
services/settings.py)_load_md_settings/_save_md_settingscovermd_export.include_front_matterandmd_export.open_after_export; wired intoload_settings/_save_all_settings._update_md_settingcallssettings._save_md_settings()so toggling either Markdown option now persists across restarts, matching the ODF flow.Test plan
pytest— 364 passing (+6 new):_save_md_settingswrites both keys_load_md_settingsreads both keys_save_md_settingsdefaults when unsetruff check src/ tests/cleanruff format --checkclean on touched files🤖 Generated with Claude Code