-
Notifications
You must be signed in to change notification settings - Fork 184
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
init.meta.json support #183
Conversation
Can't figure out why it's failing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! This feature was simpler than I was expecting it to be I think! :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mechanically looks pretty good, I have some more comments on organization that should be final!
server/src/rbx_snapshot.rs
Outdated
@@ -348,9 +374,29 @@ fn snapshot_imfs_directory<'source>( | |||
} | |||
} | |||
|
|||
if let Some(meta_ignore_instances) = meta.ignore_unknown_instances { | |||
snapshot.metadata.ignore_unknown_instances = meta_ignore_instances; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we bump this block to be above where we set children? We're already assigning to the snapshot's metadata and already have a change there.
server/src/rbx_snapshot.rs
Outdated
Ok(Some(snapshot)) | ||
} | ||
|
||
#[derive(Debug, Default, Serialize, Deserialize)] | ||
#[serde(rename_all = "camelCase")] | ||
struct InitMetaJson { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the Json
part of this struct name.
Since we don't serialize the struct, we might be able to drop the Serialize
derive and all of the skip_serializing_if
attributes as well.
server/src/rbx_snapshot.rs
Outdated
})? | ||
} else { | ||
InitMetaJson::default() | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can fold this code down even further!
Instead of assigning the struct out here and defaulting to InitMetaJson
, can we check for the existence of this file later, and then pull properties directly from it onto the snapshot?
Then we can localize the changes to this function to exactly one region, and limit the impact in cases that don't use an init.meta.json
file.
@LPGhatguy Done in latest commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we go!
* A minimum viable product for init.meta.json * Properties support * Add ignoreUnknownChildren support * Apply requested changes * Use reflection guiding * Add a script to the test * Change to ignoreUnknownInstances * Apply requested changes
Closes #123.
So far only supports
className
, want to know if this is what you expected before I work on this more.