search

ASPP / pelita

Actor-based Toolkit for Interactive Language Education in Python
https://github.com/ASPP/pelita_template
Other
62 stars 68 forks source link

add a new `graph` cached attribute to the `Bot` object #792

Closed otizonaizit closed 3 months ago

otizonaizit commented 4 months ago

Introduce a new cached attribute graph to the Bot object. This PR also moves the walls_to_graph function from utils.py to team.py, where it arguably now belongs. An alias walls_to_graph is left in utils.py for backward compatibility.

Fixes: #789

Debilski commented 4 months ago

Should we have a deprecated decorator on utils.walls_to_graph? At some point we should surely decide on one import only.

Should we use the frozen graph then? It doesn’t help much but at least it signals ‘do not touch’.

otizonaizit commented 4 months ago

Not sure it makes sense to have the deprecated decorator. New code will not use the old function. Old code is not going to be updated by anyone, so the deprecation won't motivate anyone to update the code. In my opinion we can leave the import there for an indefinite time.

Before merging I want to add some tests to be sure that if a bot changes the graph right now those changes are indeed lost. I am not sure given the way the caching is implemented. Sorry, I didn't think of it when I submitted the PR, otherwise I would have put the WIP label...

We can then use the frozen decorator then, which should pass the still-to-be-written tests, in case the current implementation is indeed broken.

On Wed 15 May, 07:08 +0000, Rike-Benjamin Schuppner @.***> wrote:

Should we have a deprecated decorator on utils.walls_to_graph? At some point we should surely decide on one import only.

Should we use the frozen graph then? It doesn’t help much but at least it signals ‘do not touch’.

— Reply to this email directly, view it on GitHub¹, or unsubscribe². You are receiving this because you authored the thread.☘Message ID: @.***>

––––

¹ https://github.com/ASPP/pelita/pull/792#issuecomment-2112653689 ² https://github.com/notifications/unsubscribe-auth/AACUYC4UBHUZJYSAE2XWV2TZCNT6VAVCNFSM6AAAAABHX4IJ5GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJSGY2TGNRYHE

Debilski commented 4 months ago

Ok but if we keep the import around then we should still flag it so that is is not removed by some import checker. Or remove it now and live with the consequences.

otizonaizit commented 4 months ago

The graph is now a read-only view and tests have been added. Ready to merge on my side.