-
Notifications
You must be signed in to change notification settings - Fork 7
Add rebus internal splits for perfect round-trip conversion #59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Addresses pipe positioning bug where splits within rebus squares were incorrectly positioned during xd → JSON → xd conversion. Previously, `JUS[T|A|SK]OSH` would convert to `JUS[TASK]|O|SH` instead of the correct `JUST|A|SKOSH`. - **Extended Clue interface** with `rebusInternalSplits?: Record<number, number[]>` to track splits within rebus squares by tile index and character positions - **Rewrote `parseSplitsFromAnswer`** to distinguish between tile boundary splits and internal rebus splits, mapping character positions to tile positions - **Completely refactored `resolveFullClueAnswer`** to process tiles individually, applying internal splits within rebus words while preserving tile boundary splits Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a pipe positioning bug in rebus squares during xd → JSON → xd round-trip conversion by implementing proper tracking and reconstruction of splits within rebus squares.
- Extended the Clue interface to support tracking internal rebus splits
- Completely rewrote the split parsing logic to distinguish between tile boundary and internal rebus splits
- Refactored
resolveFullClueAnswer
to handle internal rebus splits during answer reconstruction
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
packages/xd-crossword-tools-parser/src/types.ts | Added rebusInternalSplits field to Clue interface for tracking splits within rebus squares |
packages/xd-crossword-tools-parser/src/parser/xdparser2.ts | Rewrote parseSplitsFromAnswer to distinguish tile boundary vs internal rebus splits and updated clue creation |
packages/xd-crossword-tools/src/JSONtoXD.ts | Completely refactored resolveFullClueAnswer to process tiles individually and handle internal rebus splits |
packages/xd-crossword-tools/tests/JSONtoXD.test.ts | Added comprehensive test cases for rebus internal splits and perfect round-trip conversion |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Yeah, this is the same the direction my attempt went too, adding additional metadata ahead of time and then re-using that |
Also: improve tests, improve type checking in test files
dcf13f0
to
d8085c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Breaking changes are fine, we just want a major bump and a note in the changelog for implementors (e.g. TNY) |
Addresses pipe positioning bug where splits within rebus squares were incorrectly positioned during xd → JSON → xd conversion. Previously,
JUS[T|A|SK]OSH
would convert toJUS[TASK]|O|SH
instead of the correctJUST|A|SKOSH
.rebusInternalSplits
field to json representation of xd in order to handle splits inside of rebusesThis will need a major version bump.