Conversation
…nd pattern for actions, remove conditionals (SOLID baby step 2)
…rsing logic from ChristmasLights, reinforce SRP (SOLID baby step 3)
… public interface, remove direct grid access (SOLID baby step 4)
…tests by class, enforce SRP in test suite (SOLID)
…rings, type hints, and improve structure in all classes (AGENTS.md standards, TIDY FIRST)
…Position dataclass everywhere, update all tests and methods (AGENTS.md, DDD, TIDY FIRST)
…Action enum for actions, update parser, commands, and tests (AGENTS.md, DDD)
…error tests for instruction parser (AGENTS.md, TDD)
…arser validation and error messages, robust edge case handling (AGENTS.md)
…oordinate validation, improve docstring and clarity in parser (AGENTS.md)
…ction logic, improve comments and docstring, simplify parser (AGENTS.md)
… tests to use LightPosition in is_lit (interface alignment, AGENTS.md)
…rings, edge case tests, and comments to command tests (AGENTS.md)
…ocstrings and comments in LightGrid and ChristmasLights (AGENTS.md)
… raises error for invalid instructions, test robust subprocess error handling (AGENTS.md)
…sts for edge cases and robust controller flow (AGENTS.md)
….is_lit delegates to ChristmasLights.is_lit, improved encapsulation (AGENTS.md)
…tion is now immutable, improved docstring and __str__ (AGENTS.md)
…ocstrings, comments, and extensibility in instruction parser (AGENTS.md)
…dant comments and keep only essential docstrings (clean code)
There was a problem hiding this comment.
Pull request overview
This PR refactors the Christmas Lights Kata solution using modern software engineering practices, applying SOLID principles, design patterns (Command, Value Object, Factory), and Domain-Driven Design. The refactoring separates concerns by introducing proper abstractions while maintaining comprehensive test coverage.
Key changes:
- Introduced Command pattern for light operations (TurnOn, TurnOff, Toggle)
- Created value objects (
LightPosition) and enums (LightAction) for type safety - Extracted instruction parsing logic into dedicated
InstructionParserclass - Refactored
LightGridto use composition over inheritance (facade pattern) - Enhanced test coverage with new test suites for parser, commands, and controller edge cases
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
python/christmas_lights_kata/src/position.py |
Adds immutable LightPosition value object to replace tuple-based coordinates |
python/christmas_lights_kata/src/action.py |
Introduces LightAction enum to replace string-based action identifiers |
python/christmas_lights_kata/src/instruction_parser.py |
Extracts command parsing logic into dedicated parser class with validation |
python/christmas_lights_kata/src/commands.py |
Implements Command pattern with TurnOnCommand, TurnOffCommand, ToggleCommand classes |
python/christmas_lights_kata/src/controller.py |
Refactors controller to use new parser and command abstractions with error handling |
python/christmas_lights_kata/src/lights.py |
Refactors grid to use LightPosition, changes LightGrid from inheritance to composition |
python/christmas_lights_kata/tests/test_lights.py |
Updates tests to use new LightPosition API and removes legacy tests |
python/christmas_lights_kata/tests/test_instruction_parser.py |
Adds comprehensive test suite for instruction parser validation |
python/christmas_lights_kata/tests/test_controller.py |
Enhances controller tests with subprocess-based integration tests for error cases |
python/christmas_lights_kata/tests/test_commands.py |
Adds dedicated test suite for command pattern implementations |
python/christmas_lights_kata/tests/test_christmas_lights.py |
Adds integration tests for ChristmasLights class |
python/christmas_lights_kata/tests/instr.md |
Test fixture file with invalid command for testing error handling |
python/christmas_lights_kata/src/__init__.py |
Package marker file |
python/christmas_lights_kata/__init__.py |
Package marker file |
python/__init__.py |
Package marker file |
AGENTS.md |
Adds comprehensive coding guidelines and best practices documentation |
Add:, Fix:, Refactor: |
Archive files with unconventional names containing AGENTS.md content |
Comments suppressed due to low confidence (4)
python/christmas_lights_kata/src/action.py:1
- Import of 'auto' is not used.
from enum import Enum, auto
python/christmas_lights_kata/src/commands.py:1
- Import of 'Tuple' is not used.
from typing import Tuple
python/christmas_lights_kata/src/controller.py:9
- Import of 'LightPosition' is not used.
from .position import LightPosition
python/christmas_lights_kata/src/controller.py:10
- Import of 'LightAction' is not used.
from .action import LightAction
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import subprocess | ||
| import os |
There was a problem hiding this comment.
According to the AGENTS.md guidelines (line 84), inline imports should be avoided. The subprocess and os imports should be moved to the top of the file.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| instr = "turn on -1,0 through 0,0" | ||
| instr_path = tmp_path / "instr.md" | ||
| instr_path.write_text(instr) | ||
| import subprocess, os |
There was a problem hiding this comment.
According to the AGENTS.md guidelines (line 84), inline imports should be avoided. The subprocess and os imports should be moved to the top of the file. Additionally, multiple imports on the same line (comma-separated) should be on separate lines per PEP 8.
| @@ -0,0 +1,7 @@ | |||
| from enum import Enum, auto | |||
There was a problem hiding this comment.
The auto import from enum is unused. The enum values are explicitly assigned as strings rather than using auto(). Either remove the import or consider using auto() if the string values aren't required externally.
| from enum import Enum, auto | |
| from enum import Enum |
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | ||
| raise TypeError("start and end must be LightPosition instances") | ||
| grid.turn_on_region(start, end) | ||
|
|
||
| class TurnOffCommand(Command): | ||
| """Turn off lights in a region.""" | ||
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: | ||
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | ||
| raise TypeError("start and end must be LightPosition instances") | ||
| grid.turn_off_region(start, end) | ||
|
|
||
| class ToggleCommand(Command): | ||
| """Toggle lights in a region.""" | ||
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: | ||
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | ||
| raise TypeError("start and end must be LightPosition instances") |
There was a problem hiding this comment.
The runtime type checks using isinstance are unnecessary since the InstructionParser already returns LightPosition instances and Python's type system with type hints provides compile-time checking. These checks add unnecessary runtime overhead and duplicate validation. Consider removing them and relying on the type system, or if validation is truly needed, add a comment explaining why runtime checks are required.
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | |
| raise TypeError("start and end must be LightPosition instances") | |
| grid.turn_on_region(start, end) | |
| class TurnOffCommand(Command): | |
| """Turn off lights in a region.""" | |
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: | |
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | |
| raise TypeError("start and end must be LightPosition instances") | |
| grid.turn_off_region(start, end) | |
| class ToggleCommand(Command): | |
| """Toggle lights in a region.""" | |
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: | |
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | |
| raise TypeError("start and end must be LightPosition instances") | |
| grid.turn_on_region(start, end) | |
| class TurnOffCommand(Command): | |
| """Turn off lights in a region.""" | |
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: | |
| grid.turn_off_region(start, end) | |
| class ToggleCommand(Command): | |
| """Toggle lights in a region.""" | |
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: |
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | ||
| raise TypeError("start and end must be LightPosition instances") | ||
| grid.turn_on_region(start, end) | ||
|
|
||
| class TurnOffCommand(Command): | ||
| """Turn off lights in a region.""" | ||
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: | ||
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | ||
| raise TypeError("start and end must be LightPosition instances") | ||
| grid.turn_off_region(start, end) | ||
|
|
||
| class ToggleCommand(Command): | ||
| """Toggle lights in a region.""" | ||
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: | ||
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | ||
| raise TypeError("start and end must be LightPosition instances") |
There was a problem hiding this comment.
The runtime type checks using isinstance are unnecessary since the InstructionParser already returns LightPosition instances and Python's type system with type hints provides compile-time checking. These checks add unnecessary runtime overhead and duplicate validation. Consider removing them and relying on the type system, or if validation is truly needed, add a comment explaining why runtime checks are required.
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | |
| raise TypeError("start and end must be LightPosition instances") | |
| grid.turn_on_region(start, end) | |
| class TurnOffCommand(Command): | |
| """Turn off lights in a region.""" | |
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: | |
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | |
| raise TypeError("start and end must be LightPosition instances") | |
| grid.turn_off_region(start, end) | |
| class ToggleCommand(Command): | |
| """Toggle lights in a region.""" | |
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: | |
| if not isinstance(start, LightPosition) or not isinstance(end, LightPosition): | |
| raise TypeError("start and end must be LightPosition instances") | |
| grid.turn_on_region(start, end) | |
| class TurnOffCommand(Command): | |
| """Turn off lights in a region.""" | |
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: | |
| grid.turn_off_region(start, end) | |
| class ToggleCommand(Command): | |
| """Toggle lights in a region.""" | |
| def apply(self, grid: object, start: LightPosition, end: LightPosition) -> None: |
|
|
||
| def main() -> None: | ||
| """Main entry point for running the Christmas Lights Kata.""" | ||
| import argparse |
There was a problem hiding this comment.
According to the AGENTS.md guidelines (line 84), inline imports should be avoided. The import argparse should be moved to the top of the file with the other imports.
| """Should report zero lights lit for file with only comments.""" | ||
| instr_path = tmp_path / "instr.md" | ||
| instr_path.write_text("# comment\n# another comment") | ||
| import subprocess, os |
There was a problem hiding this comment.
According to the AGENTS.md guidelines (line 84), inline imports should be avoided. The subprocess and os imports should be moved to the top of the file. Additionally, multiple imports on the same line (comma-separated) should be on separate lines per PEP 8.
| instr = " turn on 0,0 through 0,0 " | ||
| instr_path = tmp_path / "instr.md" | ||
| instr_path.write_text(instr) | ||
| import subprocess, os |
There was a problem hiding this comment.
According to the AGENTS.md guidelines (line 84), inline imports should be avoided. The subprocess and os imports should be moved to the top of the file. Additionally, multiple imports on the same line (comma-separated) should be on separate lines per PEP 8.
| @@ -0,0 +1,39 @@ | |||
| from typing import Tuple | |||
There was a problem hiding this comment.
The Tuple import is unused in this file. Remove it to keep imports clean.
| from typing import Tuple |
| from .position import LightPosition | ||
| from .action import LightAction |
There was a problem hiding this comment.
The imports LightPosition and LightAction are unused in this file. Remove them to keep the imports clean.
| from .position import LightPosition | |
| from .action import LightAction |
|
@micaelarp I've opened a new pull request, #2, to work on those changes. Once the pull request is ready, I'll request review from you. |
Pull Request Description: Christmas Lights Kata Refactor & SOLID/AGENTS.md Compliance
Overview
This PR refactors the entire Christmas Lights Kata codebase to fully comply with SOLID principles and the standards/practices defined in AGENTS.md. The changes improve code quality, extensibility, maintainability, and testability, making the kata an ideal base for refactoring exercises, SOLID workshops, and clean design practices.
Main Changes
_grid).LightPosition).Impact and Motivation
These guidelines are designed not only to ensure technical excellence, but also to foster a culture of learning, collaboration, and continuous improvement. By applying AGENTS.md, the team has already achieved significant improvements in code quality and maintainability (e.g., the Christmas Lights Kata refactor). This foundation will help onboard new contributors, streamline code reviews, and support future growth and adaptation as the project evolves.
Justification
Conclusion
This PR transforms the kata into an example of clean, robust, and extensible code, aligned with SOLID and AGENTS.md, ready to be used in workshops, refactoring exercises, and as a base for new katas or features.