Skip to content

Environment's add paths leave the environment ID set when the grid declines the placement #47

Description

@dmccoystephenson

Problem

Environment.addEntity (src/environment.py:49) and Environment.addEntityToLocation (src/environment.py:54) both set the entity's environment ID before delegating to the grid. Now that PR #46 made the grid's add paths decline gracefully instead of raising, a declined placement returns normally and the environment ID is left set on an entity that was never placed.

Both cases were reproduced against the current main rather than inferred:

env = Environment("test", 2)
entity = Entity("test")
location = Location(9, 9)          # never added to the environment's grid

env.addEntityToLocation(entity, location)
# Warning: A location was not present when attempting to add an entity to it in a grid.

entity.getEnvironmentID()   # the environment's ID — set before the grid declined
entity.getGridID()          # -1
env.isEntityPresent(entity) # False
env = Environment("empty", 0)      # a zero-sized environment is constructible
entity = Entity("test")

env.addEntity(entity)
# Warning: A grid had no locations when attempting to add an entity to it.

entity.getEnvironmentID()   # the environment's ID
entity.getGridID()          # -1
env.getNumEntities()        # 0

The result is an entity whose three containment IDs disagree: it claims membership in an environment, denies membership in that environment's grid, and is present in neither.

Why this matters

This is the same partial-mutation shape as case 1 of #45, one layer up. Before PR #46 it was masked, because the grid raised and the caller never saw a normal return; the exception at least signalled that something had gone wrong. Now the failure is silent from the environment's point of view.

Environment.removeEntity (src/environment.py:59) already gets the ordering right — it checks self.grid.isEntityPresent(entity) first and only clears the environment ID after the grid removal succeeds. The add side is the remaining asymmetry.

Suggested fix

The environment ID should be set only once the grid has actually accepted the entity, mirroring how removeEntity clears it only after a successful removal. Whether that is expressed as a pre-check (the grid has locations / the given location is in the grid) or as a check of self.grid.isEntityPresent(entity) after delegating is worth deciding deliberately, since the second approach keeps the membership knowledge inside Grid rather than duplicating it in Environment.

Acceptance criteria

  • Adding an entity to a location that is not in the environment's grid does not leave the entity's environment ID set.
  • Adding an entity to an environment whose grid has no locations does not leave the entity's environment ID set.
  • Both paths are covered by a test for the accepted and the declined case.

This issue body was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions