Add IDs to common objects #592
Conversation
fixes [sc-12241]
|
This pull request has been linked to Shortcut Story #12241: Add IDs to CLI output. |
src/utils.rs
Outdated
| if let Ok(url) = Url::parse(url) { | ||
| let path_segments: Vec<_> = url.path_segments().unwrap().collect(); | ||
| if let Some(uuid_segment) = path_segments.get(3) { | ||
| if uuid_segment.len() == 36 { |
There was a problem hiding this comment.
Might be a little cleaner to define a constant here like UUID_LENGTH OR UUID_SEGMENT_LENGTH instead of the raw 36.
There was a problem hiding this comment.
Thanks for taking a look!
I've had a chance to think about this a bit too, this is too specific to be in the utils file. If I tried using it for something other than projects it would fail or be inconsistent since we string ids and the ids, for example parameters, could be at another position in the vector.
I think having the parent project's id would be useful, but it may not be worth it unless you or someone else has some advice.
There was a problem hiding this comment.
Also, you are correct, that's a better way re: constant
There was a problem hiding this comment.
@dwdozier This approach works fine for the CLI because missing/empty values are treated as empty strings. However, if you wanted to use this function more generally, the idiomatic Rust approach would be to use an Option<String>.
Then, on the caller side, you can convert to a String using .unwrap_or_default(). This will unwrap the option, or use String::default() when it's None. the default method for String will just return an empty string.
As an added bonus, doing it the idiomatic way makes the code much simpler.
src/utils.rs
Outdated
| pub fn get_uuid_from_url(url: &str) -> String { | ||
| if let Ok(url) = Url::parse(url) { | ||
| let path_segments: Vec<_> = url.path_segments().unwrap().collect(); | ||
| if let Some(uuid_segment) = path_segments.get(3) { | ||
| if uuid_segment.len() == 36 { | ||
| uuid_segment.to_string() | ||
| } else { | ||
| "".to_string() | ||
| } | ||
| } else { | ||
| "".to_string() | ||
| } | ||
| } else { | ||
| "".to_string() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
| pub fn get_uuid_from_url(url: &str) -> String { | |
| if let Ok(url) = Url::parse(url) { | |
| let path_segments: Vec<_> = url.path_segments().unwrap().collect(); | |
| if let Some(uuid_segment) = path_segments.get(3) { | |
| if uuid_segment.len() == 36 { | |
| uuid_segment.to_string() | |
| } else { | |
| "".to_string() | |
| } | |
| } else { | |
| "".to_string() | |
| } | |
| } else { | |
| "".to_string() | |
| } | |
| } | |
| pub fn get_uuid_from_url(url: &str) -> Option<String> { | |
| let url = Url::parse(url).ok()?; | |
| let path_segments: Vec<_> = url.path_segments()?.collect(); | |
| let uuid_segment = path_segments.get(3)?; | |
| if uuid_segment.len() == 36 { | |
| Some(uuid_segment.to_string()) | |
| } else { | |
| None | |
| } | |
| } | |
There was a problem hiding this comment.
Nice! Thanks Adam, good to hear from you! Hope the new gig is going great!
I will surely do this. I went down this path at first, but couldn't reconcile the type I was sending into the caller (and Option return type) and got it to the point where "it works" in hopes that the review would shake out the fumbling-my-way-through-it approach! ;)
Also, I still feel it's not quite a useful utility function in that it's quite specific to the URL pattern "http:///<api_path>/<api_version>///" where the UUID has to be in the 3 position, but I also don't care to overcomplicate it, so moving the function into the projects file seems prudent, especially since that's all I intended for this "feature"?
- Removed stdout table comparisons
- Checking for the presence of values instead of
actual formatting
fixes [sc-12241]
Got a little more into it than I had originally intended.