Simplify PrepromptsHolder to just be a function

[AntonOsika]

Currently preprompt holders is this:

class PrepromptsHolder:
    def __init__(self, preprompts_path: Path):
        self.preprompts_path = preprompts_path

    def get_preprompts(self) -> Dict[str, str]:
        preprompts_repo = DiskMemory(self.preprompts_path)
        return {file_name: preprompts_repo[file_name] for file_name in preprompts_repo}

We could change it to just a function:

def get_preprompts(preprompts_path: Path) -> Dict[str, str]:
    return dict(DiskMemory(preprompts_path).items())

[TheoMcCabe]

There are 6 or so uses of preprompts_holder.get_preprompts() . It seems to me if we made this change we would have to pass the preprompts path around everywhere so that we could call this new function with the path as the argument. I think its better how it is, setting the path once in this singleton class which can then be reused all over the place without needing to know the path. Especially as in the future there may be use cases where we might want to override the preprompts holder with a new class which retrievs them from a database or something

[ATheorell]

I agree with @TheoMcCabe .

[AntonOsika]

Are you sure? @ATheorell @TheoMcCabe

Now we're passing preprompts_holder around. Which is weird.

I'm suggesting we call the function immediately, and pass Dict[str, str] with preprompts around.

This means that "file access" fails early, which is better in this case (file access will be so fast user will never notice).

[TheoMcCabe]

Ah yeah - I was wondering why the file system was accessed at get time rather than in the construction of the preprompts holder. Your solution of retrieving the preprompts early and passing them in memory sounds good.

We could also leave most of the code the same and retrieve the dictionary in the init of the preprompts_holder and then return this dict when get is called ? Seems equivalent to what your suggesting. But yeah as long as we are passing around an object or a dict not a file path thats fine for me

[ATheorell]

I see that there could be a gain in fail early, but of all things that needs to be done/refactored/implemented, this looks like very low priority to me. This post in ruby on rails highlights the hidden costs of cosmetic changes: rails/rails#13771 (comment)

When I originally made the prepromptsholder, I wanted it to be general and allow for advanced editing of the preprompts during runtime, in case this would be requested when using gpt-engineer through a GUI.

All this said, if you really think this is priority and someone wants to implement it and someone else wants to supervise that and check the solution before merging, I'm not gonna hinder this development.