Conversation
- Moved `symbol.py` to `src/abyssal_tome/symbol.py`. - Consolidated constants by adding `wild` and `fast` to `src/abyssal_tome/constants.py`. - Updated `scripts/process_new_format.py` to import from `abyssal_tome.symbol`. - Restored missing `replace_special_tags` function body in `src/abyssal_tome/app.py`. - Fixed syntax errors in `scripts/process_new_format.py` (indentation) and `scripts/process_json.py` (docstrings/regex). - Updated `src/abyssal_tome/app.py` to use `constants` for Flet path/port and handle `clipman.init()` gracefully. - Fixed `tests/test_process_new_format.py` to work with current codebase (imports, models, enum). - Applied `ruff format` and `ruff check --fix` (including `UP` rules for type hints). - Added `scripts/__init__.py` to allow importing scripts in tests.
- Moved `symbol.py` to `src/abyssal_tome/symbol.py`. - Consolidated constants and regex patterns in `src/abyssal_tome/constants.py`. - Updated `src/abyssal_tome/app.py` to import constants and patterns from `constants.py`, fixing NameErrors. - Restored missing `replace_special_tags` function body in `src/abyssal_tome/app.py`. - Fixed syntax errors in `scripts/process_new_format.py` and `scripts/process_json.py`. - Updated `scripts/process_new_format.py` to use new import paths and list types. - Fixed `tests/test_process_new_format.py` to work with current codebase (imports, models). - Added `scripts/__init__.py` to facilitate testing. - Applied `ruff format` and `ruff check --fix` (including `UP` rules). - Removed accidental binary index files.
- Moved `symbol.py` to `src/abyssal_tome/symbol.py`. - Consolidated constants and regex patterns in `src/abyssal_tome/constants.py`. - Updated `src/abyssal_tome/app.py` to import constants and patterns from `constants.py`, fixing NameErrors. - Restored missing `replace_special_tags` function body in `src/abyssal_tome/app.py`. - Fixed syntax errors in `scripts/process_new_format.py` and `scripts/process_json.py`. - Updated `scripts/process_new_format.py` to use new import paths and list types. - Fixed `tests/test_process_new_format.py` to work with current codebase (imports, models). - Added `scripts/__init__.py` to facilitate testing. - Applied `ruff format` and `ruff check --fix` (including `UP` rules). - Removed accidental binary index files.
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors imports and applies consistent code formatting across the codebase. The main changes consolidate constants, remove unused imports, and apply Black-style formatting to improve code readability and maintainability.
Key changes:
- Centralized TAG_TO_LETTER and regex patterns into constants.py to eliminate duplication
- Removed unused imports (requests, open_dir, lists, sampled_from)
- Applied Black-style multi-line formatting to long function signatures and calls
- Updated test signatures to match new API (added parameters to process_ruling_html calls)
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_replace_special_tags.py | Updated import path from main to abyssal_tome.app; reformatted assertion |
| tests/test_process_new_format.py | Removed unused hypothesis imports; updated test calls with new parameters; removed obsolete test function |
| src/abyssal_tome/symbol.py | Replaced local TAG_TO_LETTER with import from constants |
| src/abyssal_tome/model.py | Applied multi-line formatting to function signatures |
| src/abyssal_tome/constants.py | Added regex import and centralized TAG_TO_LETTER and pattern constants |
| src/abyssal_tome/app.py | Removed unused imports; moved patterns to constants; added clipman error handling; applied extensive multi-line formatting |
| scripts/process_new_format.py | Updated symbol imports to absolute path; commented out duplicate TAG_TO_LETTER |
| scripts/process_json.py | Fixed docstring formatting; removed duplicate code; improved indentation |
| scripts/enrich_rulings_ai.py | Reordered imports; simplified list conversions using sorted() directly on sets |
| process_json_to_SQLite.py | Added pathlib import; replaced open() with Path.open(); removed blank line |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -74,9 +74,9 @@ def test_replace_special_tags( | |||
| spans = replace_special_tags(page, ruling_text) | |||
There was a problem hiding this comment.
The function replace_special_tags is async (as defined in src/abyssal_tome/app.py:183), but it's being called without await. This test function should be marked as async and use await when calling replace_special_tags, or use asyncio.run() to execute the async function.
| "fast": "j", | ||
| "wild": "z", |
There was a problem hiding this comment.
[nitpick] The keys 'fast' and 'wild' are added at the end of TAG_TO_LETTER dictionary, breaking alphabetical ordering. The dictionary was previously ordered with symbols like 'willpower', 'agility', 'combat', etc. in a logical grouping. Consider maintaining consistent ordering by placing these entries with their related groups or in alphabetical order.
| combined_codes.discard(source_card_code) | ||
| return sorted(list(combined_codes)) | ||
| return sorted(list(set(existing_related_codes))) | ||
| return sorted(combined_codes) |
There was a problem hiding this comment.
[nitpick] The simplified sorted(combined_codes) is correct since combined_codes is already a set. However, for consistency with line 77, consider making it explicit that you're sorting a set: sorted(list(combined_codes)). While not technically necessary, it makes the code more uniform and the intent clearer.
| return sorted(combined_codes) | |
| return sorted(list(combined_codes)) |
|
|
||
| Removes formatting artifacts, extracts FAQ references, reformats update timestamps, and determines the entry type (erratum, question/answer, or clarification). Returns a structured dictionary with cleaned content, source metadata, card name, and card code, or None if the ruling is empty or overruled. | ||
| """ |
There was a problem hiding this comment.
The docstring is missing its closing triple quotes on a new line. The closing \"\"\" should be on line 91 (after the description), not line 90. Currently, line 90 has the closing quotes immediately after 'overruled.' which is inconsistent with the multi-line docstring format used elsewhere in this file and the codebase.
| """ | ||
| def _create_copy_button_lambda(btn_ruling_text, btn_ruling_question, btn_ruling_answer, btn_instance: ft.IconButton): | ||
|
|
||
| def _create_copy_button_lambda( |
There was a problem hiding this comment.
Variable _create_copy_button_lambda is not used.
| def _create_copy_button_lambda_for_card_view( | ||
| btn_ruling_text, btn_ruling_question, btn_ruling_answer, btn_instance: ft.IconButton | ||
| ): | ||
| rules_text_content = ( | ||
| btn_ruling_text or rf"Q: {btn_ruling_question}\n A: {btn_ruling_answer}" | ||
| ) | ||
| return lambda e: asyncio.create_task( | ||
| copy_ruling_to_clipboard(e, rules_text_content, btn_instance) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Variable _create_copy_button_lambda_for_card_view is not used.
| def _create_copy_button_lambda_for_card_view( | |
| btn_ruling_text, btn_ruling_question, btn_ruling_answer, btn_instance: ft.IconButton | |
| ): | |
| rules_text_content = ( | |
| btn_ruling_text or rf"Q: {btn_ruling_question}\n A: {btn_ruling_answer}" | |
| ) | |
| return lambda e: asyncio.create_task( | |
| copy_ruling_to_clipboard(e, rules_text_content, btn_instance) | |
| ) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -175,30 +159,94 @@ async def highlight_spans(text_spans: list[ft.TextSpan], search_term: str) -> li | |||
| return highlighted_spans | |||
There was a problem hiding this comment.
Remove await on synchronous highlight_text
highlight_spans awaits highlight_text, but highlight_text is a normal function returning a list, not a coroutine. As soon as a search term triggers update_search_view and calls highlight_spans, Python raises TypeError: object list can't be used in 'await' expression, preventing any search results from rendering. Make highlight_text async or drop the await so searches can complete.
Useful? React with 👍 / 👎.
No description provided.