gpt-engineer-org / gpt-engineer

Platform to experiment with the AI Software Engineer. Terminal based. NOTE: Very different from https://gptengineer.app
MIT License
52.14k stars 6.79k forks source link

DB class was renamed to FileRepository, but docs and comments still refer to DB class #836

Closed fabhed closed 11 months ago

fabhed commented 11 months ago

Discussion: Should we have this rename? DB was quite a simple and general name.

If we are set on the new name we should update docs and comments.

TheoMcCabe commented 11 months ago

Discussion: Should we have this rename? DB was quite a simple and general name.

If we are set on the new name we should update docs and comments

So the reason i made this change is that we have added a new type of database to the project - an in memory vector store - therefore DB would now not be a descriptive enough name to distinguish it from the vector store. I also prefer specific names over generic names especially as DB implies this class possibly defines or adds a wrapper around an actual database technology e.g. sql rather than just edit files on an operating system

This was just my instinct when coding it, to make the code more readable.

TheoMcCabe commented 11 months ago

Heres Chat GPTs opinion the name change

Question: This class was renamed from DB to FileRepository - which in your opinion is a better name for the class? ---all code for the file---

Answer: Both DB and FileRepository are indicative names for the class described in the code you've presented, but they suggest different scopes and functionalities. The choice between the two names should reflect the intended use and behavior of the class as understood within the context of its application. Let's analyze both options:

DB (Database)

FileRepository

My Opinion

FileRepository seems to be a more fitting name given the context. It reduces ambiguity by specifying the nature of the storage (file-based) and the abstraction level (repository pattern), which suggests a collection of files where the file system is being used as the storage mechanism.

The class is designed for file-based key-value storage where the keys are filenames and the values are file contents. If the class's purpose within the system is indeed to abstract the file system operations and provide a simple key-value interface to files, then FileRepository accurately reflects its role.

It is also worth noting that the term "repository" is often used in domain-driven design to represent a collection of domain objects that are stored and retrieved, which can include file-based persistence mechanisms.

Overall, FileRepository helps to make the code more self-documenting and can make it easier for new developers to understand the purpose of the class at a glance. This is generally considered a best practice in software engineering—choosing names that clearly express the intent and functionality of the class.

TheoMcCabe commented 11 months ago

If we are happy with this change - I will go ahead and update the docs- apologies that I missed this!

TheoMcCabe commented 11 months ago

I also asked chat gpt if it could improve the name. It suggested an option could be FileKeyValueStore ... Its not sexy is it :)

ATheorell commented 11 months ago

It may have been easier to stick to the original naming, given that we have an upcoming high priority refactor. After refactoring, I believe we should have an abstract db as a base with a simple name and more descriptive names of the classes implementing it.

For now, for compatibility, I would support reverting the name change until we do the refactor.

TheoMcCabe commented 11 months ago

I Disagree with reverting it. i think DB is an objectively bad name as it is unclear and unspecific. For the reasons provided above.

Even after the refactor if the implementation of the abstract interface is a file data store it should still have the name file in the class. I also don't actually think you will ever abstract away from 'Files' as a domain concept as files are what code is written and stored in

ATheorell commented 11 months ago

Closing this for now since large changes are happening anyway including discussions on naming.