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

Change rbx_reflection_database to support loading from FS #376

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

Dekkonot
Copy link
Member

As part of the effort to get the reflection database loadable locally, it's probably a good idea to have a central place that decides locations for the entire ecosystem. This will prevent consumers like Lune and Rojo from having to re-implement this logic.

The obvious choice for this, in my opinion, is rbx_reflection_database. It's a requirement for everything that uses the reflection database already, and its public API can be extended to take things from the file system into account without any issues. I've implemented it and documented it thoroughly in the README for rbx_reflection_database.

However, it also sacrifices the 'purity' of the module because it fundamentally alters its purpose. I'm okay with that, but I understand not everyone will be. Please be honest with your thoughts.

I know for sure this works on Windows and Linux. I'm assuming it will also work on MacOS, but I don't own a machine I can use for testing. I don't know how to test this in CI, so we'll just have to be mindful of changes.

Specifically, databases are now loaded from a local directory
or from an environmental variable
if either exist.
Copy link
Member

@Corecii Corecii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments and future planning concerns

rbx_reflection_database/src/lib.rs Outdated Show resolved Hide resolved
rbx_reflection_database/README.md Outdated Show resolved Hide resolved
rbx_reflection_database/src/lib.rs Outdated Show resolved Hide resolved
rbx_reflection_database/src/lib.rs Outdated Show resolved Hide resolved
rbx_reflection_database/src/lib.rs Outdated Show resolved Hide resolved
@Dekkonot
Copy link
Member Author

Allowing the database to be replaced once it's initialized is complicated because it involves mutating a static variable, so for the time being I'm going to ignore it. Tools like Rojo and Lune can work out solutions without that.

@Dekkonot Dekkonot requested a review from Corecii November 14, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants