added an option to include more than one folder from the root #665
Conversation
zleyyij
left a comment
zleyyij
left a comment
There was a problem hiding this comment.
great work, a little bit confused as to why a whole bunch of typescript was commented out, if intentional plz leave comment explaining why
| None => continue, | ||
| }; | ||
|
|
||
| // Use map_or_else instead of if let/else |
There was a problem hiding this comment.
This comment is a little bit confusing, it doesn't really explain why
|
|
||
| let candidate = doc_root.join(candidate_rel); | ||
|
|
||
| tracing::debug!("Trying candidate path: {:?}", candidate.display()); |
There was a problem hiding this comment.
By convention with the rest of the project I would import tracing::debug and shorten this line to debug!(...)
| tracing::debug!("Trying candidate path: {:?}", candidate.display()); | ||
|
|
||
| if let Some(bytes) = Self::get_file(&candidate)? { | ||
| let doc = String::from_utf8(bytes).unwrap(); // assuming valid UTF-8 files |
There was a problem hiding this comment.
There's no need to unwrap where the function returns a Result, I'd .wrap_err and propagate with ?.
If you do prefer that this is more of a strict assertion, use .expect("file must be valid utf 8")
| } | ||
| } | ||
| if children.is_empty() { | ||
| Err("No doc tree found in any doc root".to_owned()) |
There was a problem hiding this comment.
The idiomatic way to do this with color_eyre is with the bail! macro, see https://docs.rs/eyre/latest/eyre/macro.bail.html
| /// | ||
| /// EG: `./repo/docs` | ||
| doc_path: PathBuf, | ||
| doc_path: Vec<PathBuf>, |
There was a problem hiding this comment.
This was pluralized but the docstring was not, you might want to explain what multiple paths does and how it works/why you'd want multiple
| return Ok(()); | ||
| } | ||
| } | ||
| Err(format!("Document {:?} not found in any doc root", path.as_ref())) |
There was a problem hiding this comment.
bail! again https://docs.rs/eyre/latest/eyre/macro.bail.html
| assets_path: String, | ||
| ) -> Result<Self> { | ||
| let doc_path = PathBuf::from(docs_path); | ||
| let doc_path: Vec<PathBuf> = docs_path.into_iter().collect(); |
There was a problem hiding this comment.
pretty sure this line does nothing, you're iterating over a vector of pathbufs and collecting it into a vector of pathbufs. It was previously needed because the function took a string and converted it into a pathbuf but now it's just a pathbuf.
Also unsure of why the function takes Strings instead of &str, I don't remember if/why that decision was made. You typically want functions to take &str as an argument because it's cheap and ergonomic to have a reference to a string, but relatively expensive and unergonomic to heap allocate, i.e &v is coercible from &String to &str vs v.to_string()
| * If the toast is not displayed, this is set to zero. If it *is* displayed, this is the ID of the toast being rendered. | ||
| */ | ||
| let toastId = -1; | ||
| //let toastId = -1; |
| export async function renderMarkdown(input: string, output: HTMLElement): Promise<void> { | ||
| // Parse front matter and get title, description, and markdown content | ||
| getFrontMatterType(input); | ||
| // getFrontMatterType(input); |
This should allow you to add more than one folder from root, in case you have multiple doc paths, such as we do for the wiki.