Change rbx_reflection_database to support loading from FS #376
Conversation
Specifically, databases are now loaded from a local directory or from an environmental variable if either exist.
|
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. |
There was a problem hiding this comment.
Looking good, I left a few comments.
RE testing - it looks like the only code paths not covered right now are here:
rbx-dom/rbx_reflection_database/src/lib.rs
Lines 130 to 141 in d95bc80
Maybe we could add a test that simply compares the result of this to the expected path for the platform, and if we're very paranoid, add some CI targets for macOS and Windows?
| pub fn get() -> Result<&'static ReflectionDatabase<'static>, Error> { | ||
| Ok(get_local()?.unwrap_or(&BUNDLED_DATABASE)) | ||
| } |
There was a problem hiding this comment.
I'm not totally sure if breaking this interface is the right call. The way I'm thinking about it is: if get_local fails, then probably 99% of the time, users of this function will immediately want to call get_bundled, so why not just have this function do that for them? That way it does the right thing most of the time, and we can avoid breakage.
On the other hand, then it's not clear that this function can fail, and removes the ability to e.g. print a warning if the local database is corrupt while using this function.
There was a problem hiding this comment.
I'm not totally sold on breaking this either, but the other option is hiding failures from users. It might be worth taking a middleground and just emitting a message into stderr if the local database exists but is corrupted?
It may also be worth caching it if we do this though, so it doesn't print the message every time.
…` is what is expected
|
We should probably use a more generic top-level folder name (e.g. %localappdata%/rbx_dom on Windows, and ~/.rbx_dom on Mac/Linux) as it would give a better hint as to where these files came from. The reflection cache could be something like rbx_dom/cache/reflection.proto This crate might come in handy for picking directories: https://crates.io/crates/directories |
|
It's currently placed in |
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.