-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add currency #88
base: v2.0
Are you sure you want to change the base?
Add currency #88
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.
Great PR! I made a few requests for for changes before you merge, if you have any questions feel free to follow up. I like where some of this is going, a few small tweaks will get it where it needs to be!
Additionally, please try to minimize changes outside of what you detail in the PR. Making multiple smaller PRs allows us to more easily approve each one :)
@@ -81,19 +83,21 @@ def generate(filename): | |||
node_mapping: dict[str, Node] = {} | |||
n_types = { | |||
"starting": Location, | |||
"node": Node, | |||
"node": Action, |
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.
A node should be of type Node, if you want to add an option for action, you could add it as a separate key: value pair in the map
def add_twoway_connection(self, node_a: Node, node_b: Node, connection_type_a: Connection = Connection, | ||
connection_type_b: Connection = Connection): |
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.
The spacing is a little off here
self.name = start() | ||
self.current: Node = starting_location | ||
self.player = player | ||
self.name: str = None | ||
self.name: str |
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.
I think these changes belong in another PR
def __init__(self, title: str, desc: str, coins: str, player: Player): | ||
self.player = player | ||
super().__init__(title, desc) | ||
super().__init__(title, desc, coins) | ||
|
||
def on_select(self): | ||
print(self.player.summary()) | ||
self.player.coins_earned += self.coins | ||
super(Action, self).on_select() |
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.
I like how you added currency onto the Action class!
@@ -1,6 +1,10 @@ | |||
import random | |||
from pyfiglet import Figlet | |||
fonts = ["6x9", "banner", "big", "block", "bubble", "chartr", "chartri", "cour", "digital", "helv", "ivrit", "lean", "mini", "mnemonic", "sbook", "script", "shadow", "slant", "small", "smscript", "smshadow", "smslant", "standard", "term", "times", "xchartr"] | |||
|
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.
I think these changes belong in another PR
if n_type == "node": | ||
args.append(game.player) | ||
|
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.
Nodes should not affect the player. Actions, however, are built explicitly to do just that!
self.coins_earned = 0 | ||
|
||
def summary(self): | ||
return f'hp: {self.hp} xp: {self.xp}' | ||
return f'hp: {self.hp} xp: {self.xp} coins: {self.coins_earned}' |
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!
Proposed changes
Added in game currency to the game. #84
Types of changes
Added a currency system and made some changes to support it. Also refactored some of the code to make it compatible with PEP-8.