search

All-Hands-AI / OpenHands

🙌 OpenHands: Code Less, Make More
https://all-hands.dev
MIT License
31.3k stars 3.61k forks source link

[Refactor]: ShortTermHistory is confusing - refactoring could be useful #3489

Closed neubig closed 1 week ago

neubig commented 3 weeks ago

Summary

I was trying to better understand how the event stream works, and looked at ShortTermHistory, and the class is a bit confusing I think: https://github.com/All-Hands-AI/OpenHands/blob/0a3d46a90b4a390180d33381c94cc98c88e36f48/openhands/memory/history.py#L21

  1. This class inherits from list[Event], but I don't think that this list is ever used? This inheritance should probably just be removed.
  2. The relationship between ShortTermHistory and EventStream is not very clear. I'm not sure why we need two classes, and can't just use the EventStream.

I feel that this could prorbably be refactored to make the class a bit more clear and intuitive. Just starting an issue here to track this for now.

tobitege commented 3 weeks ago

For reference: grafik

enyst commented 1 week ago

Closing as duplicate of https://github.com/All-Hands-AI/OpenHands/issues/3731. Please feel free to reopen if you disagree.