-
Notifications
You must be signed in to change notification settings - Fork 0
Add named NPCs with backstories and dialogue system #45
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
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
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
Adds an interactable NPC system so players can talk to named characters in each location and read their backstories.
Changes:
- Introduced a new
NPCmodel withname,backstory, andintroduce(). - Integrated a location-specific NPC plus a “Talk to …” menu option in Bank/Docks/Shop/Tavern.
- Added/updated tests to validate NPC initialization and updated location menus.
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/npc/test_npc.py | Adds unit tests for the new NPC class initialization and introduce() formatting. |
| tests/location/test_tavern.py | Updates Tavern tests for the new NPC field and shifted menu options; adds “talk” action test. |
| tests/location/test_shop.py | Updates Shop tests for the new NPC field and shifted menu options; adds “talk” action test. |
| tests/location/test_docks.py | Updates Docks tests for the new NPC field and shifted menu options; adds “talk” action test. |
| tests/location/test_bank.py | Updates Bank tests for the new NPC field and shifted menu options; adds “talk” action test. |
| src/npc/npc.py | Adds the NPC class and introduce() method. |
| src/npc/init.py | Initializes the new npc package. |
| src/location/tavern.py | Instantiates Tavern NPC and adds dialogue option + handler. |
| src/location/shop.py | Instantiates Shop NPC and adds dialogue option + handler. |
| src/location/docks.py | Instantiates Docks NPC and adds dialogue option + handler. |
| src/location/bank.py | Instantiates Bank NPC and adds dialogue option + handler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_talkToNPC(): | ||
| # prepare | ||
| tavernInstance = createTavern() | ||
|
|
||
| # check |
Copilot
AI
Feb 1, 2026
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.
This test doesn’t call Tavern.talkToNPC(), so it won’t catch regressions in the dialogue display/pause logic. You can follow the pattern used elsewhere in this file (mock builtins.input, userInterface.lotsOfSpace/divider, and capture output) to execute talkToNPC() without blocking and assert it prints the introduction and updates currentPrompt.text.
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.
Fixed in bbf82ef - test now calls talkToNPC() with mocked showDialogue and verifies the introduction text is passed correctly.
| def test_talkToNPC(): | ||
| # prepare | ||
| docksInstance = createDocks() | ||
|
|
||
| # check |
Copilot
AI
Feb 1, 2026
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.
test_talkToNPC currently only checks the NPC data and doesn’t execute Docks.talkToNPC(). Since the suite already mocks builtins.input/stdout for other interactive methods, consider calling talkToNPC() with builtins.input mocked (and capturing stdout) to verify it prints the introduction and resets currentPrompt.text.
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.
Fixed in bbf82ef - test now executes talkToNPC() with mocked showDialogue and asserts the introduction is passed correctly.
| def test_talkToNPC(): | ||
| # prepare | ||
| bankInstance = createBank() | ||
|
|
||
| # check |
Copilot
AI
Feb 1, 2026
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.
test_talkToNPC doesn’t run Bank.talkToNPC()—it only verifies the NPC data. Consider mocking builtins.input (so it won’t block), stubbing userInterface.lotsOfSpace/divider, and asserting the introduction is printed (e.g., with capsys) and currentPrompt.text is reset.
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.
Fixed in bbf82ef - test now runs talkToNPC() with mocked showDialogue and verifies the introduction text and method call.
| from prompt.prompt import Prompt | ||
| from world.timeService import TimeService | ||
| from ui.userInterface import UserInterface | ||
| from npc.npc import NPC |
Copilot
AI
Feb 1, 2026
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.
In this file the Stats type hint is coming from pstats (line 1), which is a different class than the game’s stats.stats.Stats used elsewhere (e.g., src/location/shop.py:6). Since this PR touches the import block, please switch the Stats import here to from stats.stats import Stats for consistency and correct typing.
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.
Fixed in bbf82ef - changed import from pstats.Stats to stats.stats.Stats for correct typing.
src/location/tavern.py
Outdated
| def talkToNPC(self): | ||
| self.userInterface.lotsOfSpace() | ||
| self.userInterface.divider() | ||
| print(self.npc.introduce()) | ||
| self.userInterface.divider() |
Copilot
AI
Feb 1, 2026
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.
talkToNPC() is identical across multiple locations (Bank/Docks/Shop/Tavern). Consider extracting this into a shared helper (e.g., a UserInterface.showDialogue(text) method or a small utility function) to avoid keeping four copies in sync if the dialogue UI changes.
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.
Fixed in bbf82ef - extracted talkToNPC() into UserInterface.showDialogue() method to eliminate duplication across all four locations.
tests/location/test_shop.py
Outdated
| # call | ||
| # We can't fully test the input() part, but we can test the method exists | ||
| # and the NPC has the right data | ||
| assert shopInstance.npc.name == "Gilbert the Shopkeeper" | ||
| assert len(shopInstance.npc.backstory) > 0 |
Copilot
AI
Feb 1, 2026
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.
This test doesn’t exercise Shop.talkToNPC()—it only asserts the NPC data. Since other tests in this suite already patch builtins.input, you can unit-test the method by mocking builtins.input to return immediately and asserting the printed introduction (e.g., via capsys) and prompt update.
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.
Fixed in bbf82ef - test now exercises talkToNPC() with mocked showDialogue and asserts the introduction text is passed correctly.
|
@copilot address comments |
…ace, improve tests Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Expand NPC Backgrounds - Implementation Complete ✅
Summary
Successfully implemented a comprehensive NPC system for the FishE fishing village game. Each location now has a unique named NPC with a detailed backstory that players can discover through dialogue.
NPCs Added:
Recent Changes (addressing PR feedback):
Implementation Details:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.