Add writeNewModelAsset and writeNewPlaceAsset #40
Conversation
|
I don't have any good ideas on how to format the |
|
Alright, they now return the asset id. I originally tried to combine |
src/remodel_api/remodel.rs
Outdated
| fn get_required_string_option(options: &Table, option: &str) -> rlua::Result<String> { | ||
| let value = options.get(option).map_err(rlua::Error::external)?; | ||
|
|
||
| match value { | ||
| Value::String(value) => Ok(value.to_str().map_err(rlua::Error::external)?.to_string()), | ||
| Value::Nil => Err(rlua::Error::external(format!( | ||
| "The option {} must be specified", | ||
| option | ||
| ))), | ||
| _ => Err(rlua::Error::external(format!( | ||
| "The option {} must be a string", | ||
| option | ||
| ))), | ||
| } | ||
| } | ||
|
|
||
| fn get_string_option(options: &Table, option: &str, default: &str) -> rlua::Result<String> { | ||
| let value = options.get(option).map_err(rlua::Error::external)?; | ||
|
|
||
| match value { | ||
| Value::String(value) => Ok(value.to_str().map_err(rlua::Error::external)?.to_string()), | ||
| Value::Nil => Ok(default.to_string()), | ||
| _ => Err(rlua::Error::external(format!( | ||
| "The option {} must be a string", | ||
| option | ||
| ))), | ||
| } | ||
| } | ||
|
|
||
| fn get_bool_option(options: &Table, option: &str, default: bool) -> rlua::Result<bool> { | ||
| let value = options.get(option).map_err(rlua::Error::external)?; | ||
|
|
||
| match value { | ||
| Value::Boolean(value) => Ok(value), | ||
| Value::Nil => Ok(default), | ||
| _ => Err(rlua::Error::external(format!( | ||
| "The option {} must be a bool", | ||
| option | ||
| ))), | ||
| } | ||
| } |
There was a problem hiding this comment.
These functions feel pretty heavy! We should be able to write one generic function for required parameters and one for optional ones. We can leverage the FromLua trait to make this work for any type and avoid a lot of boilerplate.
src/remodel_api/remodel.rs
Outdated
| fn bool_into_query(boolean: bool) -> String { | ||
| if boolean { | ||
| String::from("True") | ||
| } else { | ||
| String::from("False") | ||
| } | ||
| } |
There was a problem hiding this comment.
Usage of this should not be pervasive enough to warrant a function; we should be able to keep the bool-to-query stuff entirely inside the code that actually does HTTP requests.
src/remodel_api/remodel.rs
Outdated
| context: Context<'_>, | ||
| lua_instance: LuaInstance, | ||
| asset_id: u64, | ||
| ) -> rlua::Result<()> { | ||
| queries: &[(&str, &str)], |
There was a problem hiding this comment.
I don't think this is the right interface here. We should instead accept a struct of known fields that are turned into the correct querystring parameters by this function.
src/remodel_api/remodel.rs
Outdated
| match Remodel::write_model_asset(context, lua_instance, asset_id, &[]) { | ||
| Ok(_) => Ok(()), | ||
| Err(error) => Err(rlua::Error::external(error)), | ||
| } |
There was a problem hiding this comment.
This function already returns an rlua::Result, we can just return the error here, or better yet, use Rust's ? operator.
src/remodel_api/remodel.rs
Outdated
| match Remodel::write_place_asset(context, lua_instance, asset_id, &[]) { | ||
| Ok(_) => Ok(()), | ||
| Err(error) => Err(rlua::Error::external(error)), | ||
| } |
There was a problem hiding this comment.
Same here as in the other case. Matching here doesn't really make sense when we have ?.
src/remodel_api/remodel.rs
Outdated
| buffer: Vec<u8>, | ||
| asset_id: u64, | ||
| queries: &[(&str, &str)], | ||
| ) -> rlua::Result<String> { |
There was a problem hiding this comment.
This return value should probably be a u64, not a string, since asset IDs are numbers.
src/remodel_api/remodel.rs
Outdated
| let asset_id = response.headers().get("roblox-assetid"); | ||
|
|
||
| match asset_id { | ||
| Some(asset_id) => Ok(String::from(asset_id.to_str().unwrap())), |
There was a problem hiding this comment.
I don't know if we can unwrap here. We don't control this value, so whatever can cause to_str to not return a value might fail!
src/remodel_api/remodel.rs
Outdated
| context: Context<'lua>, | ||
| options: &Table<'lua>, | ||
| option: &str, | ||
| default: T, |
There was a problem hiding this comment.
Let's just make this function return an rlua::Result<Option<T>> instead of passing a default. Rust has lots of great features that let us easily fall back to a default that are more flexible than this. We can also merge it with get_required_option for the same reason!
src/remodel_api/remodel.rs
Outdated
| }; | ||
|
|
||
| let description = get_option(context, &options, "description")?; | ||
| let description = match description { |
There was a problem hiding this comment.
For all of these, I think you want .unwrap_or_default() instead for succinctness.
|
Close in favor of #79 |
Pretty straightforward implementation. All options are not required except for the
nameoption. The other ones default to""orfalse. Technically,allowCommentsdoesn't change any behavior for places, but I left it in there for future proofing? I can remove it if necessary.