Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a method to list all boxes #1230

Closed
wants to merge 1 commit into from

Conversation

Louis-Navarro
Copy link

Resolves #1122 by givin the user an easy way to get a list with the names of all the boxes

@themisir
Copy link
Contributor

themisir commented Aug 10, 2023

This only lists all the open boxes. It may confuse the users with the assumption that it lists all the boxes that's been created, including the closed ones. Technically we can list them as well, however the implementation will not be reliable, we would have to list entries of the directory Hive has been initialized on, which may return false positives.

I also find this method counter intuitive. While it helps developers with certain needs, I believe this part should be out of scope of the database. It's users' responsibility to maintain resources they're using, not the library. Even if the library has an internal structure holding the list of temporarily open boxes, that may change in the future and the change would be limited by the current decisions if we were to make one. I'm rather on the side of keeping exposed API interface as small as possible while providing backwards compatible updates.

Aside from my personal opinion, Simon is planning to update hive, which may possibly make lots of changes to the internal implementation considering he's planning to completely change the storage implementation. I think at this point it would be more sensible to not extend the API surface and just focus on what we've already got in hand.

I hope you wouldn't find this unmotivating. If you think the change must be done, I am open to discussing this with other maintainers.

@Louis-Navarro
Copy link
Author

Thank you for the feedback. I understand this feature is not aligned with the project's overall direction and goals at this time. While I believe it could be useful, I agree it makes sense to keep the API surface small and focus on backwards compatible updates for now. Especially with big changes planned for the storage implementation. I'm happy to close this pull request and discuss other ways I could contribute that would be more in line with where things are headed. Appreciate you taking the time to explain your perspective, it's helpful for me to understand the reasoning behind decisions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a listBoxes() method
2 participants