Skip to content
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

DoorSpaceRandomizer #35

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

DoorSpaceRandomizer #35

wants to merge 7 commits into from

Conversation

JamesDeVore
Copy link
Collaborator

This is the first pass for creating the DoorSpaceRandomizer class.
It is not complete, but I just want feedback on progress so far. Tests still need to be written, etc

  • Main question is: Why take in a DoorSpace (as defined in the issue) as a parameter if a new one will be returned anyway?

Copy link
Owner

@tmcclintock tmcclintock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely going in the right direction. Most of the comments are formatting related. Be sure to read the developing section that deals with installing pre-commit so that you don't have to worry about formatting it yourself by hand.

@@ -1,7 +1,7 @@
import random
from typing import Optional

from donjuan import Cell, Grid
from donjuan import Cell, Grid, DoorSpace, Door,Portcullis,Archway
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

black formatting and isort

@@ -16,6 +16,9 @@ def randomize_cell(self, cell: Cell) -> None:
def randomize_grid(self, grid: Grid) -> None:
"""Randomize properties of the `Grid`"""
pass # pragma: no cover
def randomize_door(self, cell: DoorSpace):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def randomize_door(self, cell: DoorSpace):
def randomize_door(self, cell: DoorSpace):

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the argument should be cell is should be called door_space.

@@ -41,3 +44,30 @@ def randomize_grid(self, grid: Grid) -> None:
for j in range(grid.n_cols):
self.randomize_cell(grid.cells[i][j])
return
class DoorSpaceRandomizer(Randomizer):
"""
Randomly set the :attrs: of the DoorSpace and select a DoorSpace type (Door, Archway, Portcullis).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Randomly set the :attrs: of the DoorSpace and select a DoorSpace type (Door, Archway, Portcullis).
Randomly set the attributes of the DoorSpace subclass (Door, Archway, Portcullis).

def randomize_doorspace(self, door_space:DoorSpace):
#
door_types = [Door, Portcullis, Archway]
material_types = ["wood","metal","stone"]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

black formatting.

selected_type = door_types[random.randint(0,len(door_types) - 1)]()

if(isinstance(selected_type, Door)):
door_space = Door(secret=secret,locked=locked,closed=closed,jammed=jammed,blocked=blocked,broken=broken,material=material)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to replace the object with another (it wouldn't work anyway, because pass by reference). We just want to set the attributes.

Suggested change
door_space = Door(secret=secret,locked=locked,closed=closed,jammed=jammed,blocked=blocked,broken=broken,material=material)
door_space.secret=secret
door_space.locked=locked
door_space.closed=closed
door_space.jammed=jammed
door_space.blocked=blocked
door_space.broken=broken
door_space.material=material

same goes for the portcullis and archway cases.

door_space = Portcullis(locked=locked,closed=closed,jammed=jammed,broken=broken,material=material)
elif(isinstance(selected_type, Archway)):
door_space = Archway(material=material,blocked=blocked,broken=broken,secret=secret)
return door_space
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass by reference, so no return value.

Suggested change
return door_space
return

elif(isinstance(selected_type, Portcullis)):
door_space = Portcullis(locked=locked,closed=closed,jammed=jammed,broken=broken,material=material)
elif(isinstance(selected_type, Archway)):
door_space = Archway(material=material,blocked=blocked,broken=broken,secret=secret)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple if cases should be ended with an else. In this case it should raise an error, since it means we are dealing with an unknown type.

Suggested change
door_space = Archway(material=material,blocked=blocked,broken=broken,secret=secret)
door_space = Archway(material=material,blocked=blocked,broken=broken,secret=secret)
else:
raise TypeError(f"unknown type: type(door_space) = {type(door_space)")

@JamesDeVore
Copy link
Collaborator Author

Made some of the requested changes. Still have tests missing but I wanted to at least push the changes up in case anyone was waiting on it.

@JamesDeVore
Copy link
Collaborator Author

None of the tests are passing, I merged in latest main into my branch, and it's throwing all kinds of issues. So i'll keep trying to iron that out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants