-
Notifications
You must be signed in to change notification settings - Fork 41
Updated FindNoun to fix issue 356 #366
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
Conversation
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 refactors the Room.FindNoun method to address issue 356 by improving multi‑word matching, alias resolution, and plural/singular handling.
- Reordered matching logic and candidate list construction for more predictable noun matching.
- Simplified and consolidated alias resolution and pluralization/singularization checks.
- Updated unit tests to verify the corrected behavior of full‑phrase lookups and alias chaining.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/rooms/rooms_test.go | Added test cases to validate improved matching and alias resolution. |
| internal/rooms/rooms.go | Refactored alias and pluralization handling in the FindNoun method. |
Volte6
left a comment
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.
Looks good!
|
Does this handle circular references? |
Good catch, it will loop for circular aliases. Do we have an expected behavior for this case? I’d say we should catch them as early as possible. We could detect on load and log a warning or error when a cycle is found and flatten aliases at load time into a map, so runtime lookups are cycle‑free or enforce a max alias depth or maintain a visited set during runtime lookup to break cycles. I'm leaning to the detect on load as it gives the benefit of creating the noun map once and would simplify the logic FindNoun. |
|
I think just making it so no alias can point to another alias might be fine. Why would anyone want to do: When they could just do: |
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 refactors the Room.FindNoun method to improve multi‑word noun matching, alias resolution, and plural/singular handling, addressing issue #356 and updating the corresponding tests.
- Reorders matching logic to try full‑phrase lookups before individual words
- Consolidates pluralization/singularization checks and simplifies alias handling
- Adds test cases for multi‑word inputs and circular alias references
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/rooms/rooms_test.go | New test cases added for island, rocks, and circular alias handling |
| internal/rooms/rooms.go | Refactored FindNoun method with improved candidate generation, alias, and pluralization logic |
Comments suppressed due to low confidence (1)
internal/rooms/rooms.go:1633
- [nitpick] The variable name 'tn' is not very descriptive; consider renaming it to something like 'candidateNoun' for improved clarity.
tn := strings.TrimSuffix(newNoun, "es")
Description
Refactor the Room.FindNoun method to improve multi‑word noun matching, alias resolution, and plural/singular handling. This change ensures that full‑phrase lookups (e.g. “rocky island”) are tried first, aliases are unwrapped recursively, and common plural forms (…ies, …es, …s) are handled in a predictable order. It also fixes previously failing tests for “ponies” and “rocky”.
Changes