Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

Level 1 Minimum Product: Pickups #244

Merged

Conversation

OlafSzmidt
Copy link
Contributor

I decided to make multiple pull requests as the whole change of the codebase will be quite lengthy.

Changes Made:

  • HealthPickup, InvulnerabilityPickup and DamagePickup removed.
  • Slowly removing effects from the codebase, _PickupEffect from pickups.py is gone.
  • JSONDecoder changed to only decode the one type of pickup now.
  • Uses of the above removed throughout the codebase, including:
    • custom_map.py, test_pickups.py, test_simulation.py, test_pickup_decoder() , test_custom_map and ALL_PICKUPS
  • New test written.


def test_serialise(self):
self.assertEqual(self.pickup.serialise(), {'type': 'damage', 'damage_boost': 5})
self.assertEqual(self.pickup.serialise(), {'type' : 'delivery'})
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space

@OlafSzmidt
Copy link
Contributor Author

When approved, I'll clean up the history and squash these commits. Otherwise github doesn't see changes.

@@ -20,6 +20,7 @@ def __init__(self, player_id, initial_location, worker_url, avatar_appearance):
self.health = 5
self.score = 0
self.events = []
self.holdingTote = False
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure how that system will work, wouldn't it be better to have a collection of gathered pickups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought our idea was to have a single pickup and deliver it somewhere - but then that doesn't make the code very scalable if we want to change it, that's true.

Do we want to have multiple delivery items held at some particular one time?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess just having a set of how many items of which type we have at one moment would work, here that would just be a set of one tote. That would be more scalable, even if this would be easy to refactor. What do you think @OlafSzmidt @mrniket ?

super(DeliveryTote, self).__init__(cell)

def _apply(self, avatar):
avatar.holdingTote = True
Copy link
Contributor

Choose a reason for hiding this comment

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

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. I forgot to remove one of the files.

CelineBoudier
CelineBoudier previously approved these changes Sep 25, 2017
@CelineBoudier
Copy link
Contributor

Comments @mrniket before the commits are squashed?

@mrniket
Copy link
Contributor

mrniket commented Sep 25, 2017

No I'm happy :)

mrniket
mrniket previously approved these changes Sep 25, 2017
test_pickup_decoder written for the new class.

Changes made as requested in PR.

holdingTote no longer used. Implemented Counter

Obsolete pickup file removed.
@OlafSzmidt
Copy link
Contributor Author

Rebased.

@OlafSzmidt
Copy link
Contributor Author

@mrniket @CelineBoudier One last approval so we can merge this :)

@CelineBoudier
Copy link
Contributor

It's the fiiinal approooooval, tadadadaaaa

@OlafSzmidt OlafSzmidt merged commit a3cf19e into ocadotechnology:master Sep 25, 2017
@OlafSzmidt OlafSzmidt deleted the pickups_changed_for_lvl1 branch September 25, 2017 14:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants