From 7599ef6626652e1e2d4b9420bc992c9b041ac887 Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Fri, 14 May 2021 15:58:18 -0400 Subject: [PATCH] Improve error messages --- CHANGELOG.md | 1 + src/snapshot_middleware/project.rs | 131 ++++++++++++------ .../default.project.json | 7 + .../bad_classname_path_conflict/foo.txt | 0 .../bad_no_classname/default.project.json | 4 + 5 files changed, 99 insertions(+), 44 deletions(-) create mode 100644 test-projects/bad_classname_path_conflict/default.project.json create mode 100644 test-projects/bad_classname_path_conflict/foo.txt create mode 100644 test-projects/bad_no_classname/default.project.json diff --git a/CHANGELOG.md b/CHANGELOG.md index c6991f01..2d9d1ab3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * Added "EXPERIMENTAL!" label to two-way sync toggle in Rojo's Roblox Studio plugin. * Fixed `Name` and `Parent` properties being allowed in Rojo projects. ([#413][pr-413]) * Fixed "Open Scripts Externally" feature crashing Studio ([#369][issue-369]) +* Improved error messages for misconfigured projects. [issue-369]: https://github.com/rojo-rbx/rojo/issues/369 [pr-413]: https://github.com/rojo-rbx/rojo/pull/413 diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index b7468c0f..b0a56483 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -1,6 +1,6 @@ use std::{borrow::Cow, collections::HashMap, path::Path}; -use anyhow::Context; +use anyhow::{bail, Context}; use memofs::Vfs; use rbx_reflection::ClassTag; @@ -66,11 +66,13 @@ pub fn snapshot_project_node( ) -> SnapshotInstanceResult { let project_folder = project_path.parent().unwrap(); - let name = Cow::Owned(instance_name.to_owned()); - let mut class_name = node + let class_name_from_project = node .class_name .as_ref() .map(|name| Cow::Owned(name.clone())); + let mut class_name_from_path = None; + + let name = Cow::Owned(instance_name.to_owned()); let mut properties = HashMap::new(); let mut children = Vec::new(); let mut metadata = InstanceMetadata::default(); @@ -85,24 +87,7 @@ pub fn snapshot_project_node( }; if let Some(snapshot) = snapshot_from_vfs(context, vfs, &path)? { - // If a class name was already specified, then it'll override the - // class name of this snapshot ONLY if it's a Folder. - // - // This restriction is in place to prevent applying properties to - // instances that don't make sense. The primary use-case for using - // $className and $path at the same time is to use a directory as a - // service in a place file. - class_name = match class_name { - Some(class_name) => { - if snapshot.class_name == "Folder" { - Some(class_name) - } else { - // TODO: Turn this into an error object. - panic!("If $className and $path are specified, $path must yield an instance of class Folder"); - } - } - None => Some(snapshot.class_name), - }; + class_name_from_path = Some(snapshot.class_name); // Properties from the snapshot are pulled in unchanged, and // overridden by properties set on the project node. @@ -129,34 +114,66 @@ pub fn snapshot_project_node( } } - let class_name = class_name - .or_else(|| { - // If className wasn't defined from another source, we may be able - // to infer one. + let class_name_from_inference = infer_class_name(&name, parent_class); - let parent_class = parent_class?; + let class_name = match ( + class_name_from_project, + class_name_from_path, + class_name_from_inference, + ) { + // These are the easy, happy paths! + (Some(project), None, None) => project, + (None, Some(path), None) => path, + (None, None, Some(inference)) => inference, - if parent_class == "DataModel" { - // Members of DataModel with names that match known services are - // probably supposed to be those services. + // If the user specifies a class name, but there's an inferred class + // name, we prefer the name listed explicitly by the user. + (Some(project), None, Some(_)) => project, - let descriptor = rbx_reflection_database::get().classes.get(&name)?; - - if descriptor.tags.contains(&ClassTag::Service) { - return Some(name.clone()); - } - } else if parent_class == "StarterPlayer" { - // StarterPlayer has two special members with their own classes. - - if name == "StarterPlayerScripts" || name == "StarterCharacterScripts" { - return Some(name.clone()); - } + // If the user has a $path pointing to a folder and we're able to infer + // a class name, let's use the inferred name. If the path we're pointing + // to isn't a folder, though, that's a user error. + (None, Some(path), Some(inference)) => { + if path == "Folder" { + inference + } else { + path } + } - None - }) - // TODO: Turn this into an error object. - .expect("$className or $path must be specified"); + (Some(project), Some(path), _) => { + if path == "Folder" { + project + } else { + bail!( + "ClassName for Instance \"{}\" was specified in both the project file (as \"{}\") and from the filesystem (as \"{}\").\n\ + If $className and $path are both set, $path must refer to a Folder. + \n\ + Project path: {}\n\ + Filesystem path: {}\n", + instance_name, + project, + path, + project_path.display(), + node.path.as_ref().unwrap().display() + ); + } + } + + (None, None, None) => { + bail!( + "Instance \"{}\" is missing some required information.\n\ + One of the following must be true:\n\ + - $className must be set to the name of a Roblox class\n\ + - $path must be set to a path of an instance\n\ + - The instance must be a known service, like ReplicatedStorage\n\ + \n\ + Project path: {}", + instance_name, + project_path.display(), + ); + } + }; for (child_name, child_project_node) in &node.children { if let Some(child) = snapshot_project_node( @@ -230,6 +247,32 @@ pub fn snapshot_project_node( })) } +fn infer_class_name(name: &str, parent_class: Option<&str>) -> Option> { + // If className wasn't defined from another source, we may be able + // to infer one. + + let parent_class = parent_class?; + + if parent_class == "DataModel" { + // Members of DataModel with names that match known services are + // probably supposed to be those services. + + let descriptor = rbx_reflection_database::get().classes.get(name)?; + + if descriptor.tags.contains(&ClassTag::Service) { + return Some(Cow::Owned(name.to_owned())); + } + } else if parent_class == "StarterPlayer" { + // StarterPlayer has two special members with their own classes. + + if name == "StarterPlayerScripts" || name == "StarterCharacterScripts" { + return Some(Cow::Owned(name.to_owned())); + } + } + + None +} + // #[cfg(feature = "broken-tests")] #[cfg(test)] mod test { diff --git a/test-projects/bad_classname_path_conflict/default.project.json b/test-projects/bad_classname_path_conflict/default.project.json new file mode 100644 index 00000000..fc0561cc --- /dev/null +++ b/test-projects/bad_classname_path_conflict/default.project.json @@ -0,0 +1,7 @@ +{ + "name": "bad_classname_path_conflict", + "tree": { + "$className": "DataModel", + "$path": "foo.txt" + } +} \ No newline at end of file diff --git a/test-projects/bad_classname_path_conflict/foo.txt b/test-projects/bad_classname_path_conflict/foo.txt new file mode 100644 index 00000000..e69de29b diff --git a/test-projects/bad_no_classname/default.project.json b/test-projects/bad_no_classname/default.project.json new file mode 100644 index 00000000..69d02d1e --- /dev/null +++ b/test-projects/bad_no_classname/default.project.json @@ -0,0 +1,4 @@ +{ + "name": "bad_no_classname", + "tree": {} +} \ No newline at end of file