-
Notifications
You must be signed in to change notification settings - Fork 9
Filter default properties in json middlewares instead of filter fn #6
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
base: syncback-merge
Are you sure you want to change the base?
Changes from 1 commit
f635cdc
6b9e515
579f55c
b54eee1
6eb04bf
b57f473
e335caa
ea4af65
09c4e2c
7cdda8a
ca488ac
0d8a36d
583666a
cae0c9b
bcfdc0c
a854e8a
59d780c
3dd6856
a4f10c6
b2fd639
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,17 +19,21 @@ mod txt; | |
| mod util; | ||
|
|
||
| use std::{ | ||
| collections::BTreeMap, | ||
| path::{Path, PathBuf}, | ||
| sync::OnceLock, | ||
| }; | ||
|
|
||
| use anyhow::Context; | ||
| use memofs::{IoResultExt, Vfs}; | ||
| use rbx_dom_weak::{types::Variant, Instance}; | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| use crate::{ | ||
| glob::Glob, | ||
| resolution::UnresolvedValue, | ||
| syncback::{SyncbackReturn, SyncbackSnapshot}, | ||
| variant_eq::variant_eq, | ||
| }; | ||
| use crate::{ | ||
| snapshot::{InstanceContext, InstanceSnapshot, SyncRule}, | ||
|
|
@@ -388,3 +392,58 @@ pub fn default_sync_rules() -> &'static [SyncRule] { | |
| ] | ||
| }) | ||
| } | ||
|
|
||
| fn filter_default_property( | ||
| snapshot: &SyncbackSnapshot, | ||
| new_inst: &Instance, | ||
| name: &str, | ||
| value: &Variant, | ||
| attributes: &mut BTreeMap<String, UnresolvedValue>, | ||
| properties: &mut BTreeMap<String, UnresolvedValue>, | ||
|
kennethloeffler marked this conversation as resolved.
Outdated
|
||
| ) { | ||
| let db = rbx_reflection_database::get(); | ||
| let class_descriptor = db.classes.get(new_inst.class.as_str()); | ||
|
|
||
| match value { | ||
| Variant::Attributes(attrs) => { | ||
| for (attr_name, attr_value) in attrs.iter() { | ||
| // We (probably) don't want to preserve internal attributes, | ||
| // only user defined ones. | ||
| if attr_name.starts_with("RBX") { | ||
| continue; | ||
| } | ||
| attributes.insert( | ||
| attr_name.clone(), | ||
| UnresolvedValue::from_variant_unambiguous(attr_value.clone()), | ||
| ); | ||
| } | ||
| } | ||
| Variant::SharedString(_) => { | ||
| log::warn!( | ||
| "Rojo cannot serialize the property {}.{name} in JSON files.\n\ | ||
| If this is not acceptable, resave the Instance at '{}' manually as an RBXM or RBXMX.", | ||
| new_inst.class, snapshot.get_new_inst_path(new_inst.referent()) | ||
| ) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this warning also provide the path of the file where the instance is defined?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can actually include We can probably just remove this warning altogether.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mind if we tackle that in a future PR? Making that change effects several tests and I'd rather not clutter this PR with them
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we can handle that in a different PR. A bit out of scope for this one either way.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we allow rbx-dom's rbx_reflector to write defaults for SharedString properties (which is now possible after rojo-rbx/rbx-dom#414), then we won't have to alter any test data
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there's a compelling reason to not have rbx-dom write SharedString defaults now. Seems like a good change. |
||
| } | ||
| _ => { | ||
| let new_prop_is_default = if let Some(class_descriptor) = class_descriptor { | ||
| if let Some(default) = db.find_default_property(class_descriptor, name) { | ||
| variant_eq(value, default) | ||
| } else { | ||
| false | ||
| } | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| if new_prop_is_default { | ||
| properties.remove(name); | ||
|
kennethloeffler marked this conversation as resolved.
Outdated
|
||
| } else { | ||
| properties.insert( | ||
| name.to_owned(), | ||
| UnresolvedValue::from_variant(value.clone(), &new_inst.class, name), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.