Move markdown formatting from derive to runtime #455
Conversation
|
Continuation of #453 |
This eliminates the newly added `edgedb-cli-md` local crate. It also allows to disable styling if stdout isn't tty and makes formatting more flexible overall.
There was a problem hiding this comment.
Generally good. But I feel anxious about so many leak calls. Technically they are fine, as called once, but still not a good pattern. We can use static variables instead:
- let about = Box::new(crate::markdown::format_markdown(#source));
- #app = #app.about(Box::leak(about).as_str());
+ static ABOUT: ::once_cell::sync::Lazy<String> = ::once_cell::sync::Lazy::new(|| crate::markdown::format_markdown(#source)));
+ #app = #app.about(&ABOUT);Difference is that if the function is called twice we will get a cached value.
src/markdown.rs
Outdated
| if !atty::is(atty::Stream::Stdout) { | ||
| return termimad::MadSkin::no_style(); | ||
| } | ||
|
|
||
| use crossterm::style::{Color, Attribute}; |
There was a problem hiding this comment.
Please move use at the top of the block. And keep a line separating use from code. Or just move it to the module level. We usually import at function level either enums like Color::* or platform-specific stuff, or ocasionally if we need to override something.
There was a problem hiding this comment.
Sure, moved to the top of the block. I'm also fine to move it to the top of the module, my motivation to keep it in the function was that those names are only used in it, and the function is like 10% of the file. But again, I'm all for consistent code style so let me know if you want to move it to the module level.
src/markdown.rs
Outdated
| @@ -23,18 +23,24 @@ pub fn prepare_markdown(text: &str) -> String { | |||
| return buf; | |||
| } | |||
|
|
|||
| pub fn make_skin() -> termimad::MadSkin { | |||
| fn make_skin() -> termimad::MadSkin { | |||
There was a problem hiding this comment.
Let's store MadSkin in a once_cell::sync::Lazy. As even if MadSkin constructor is fast enough, we do a syscall in the function (atty::is). Doing that per each piece of text may not be very noticeable on fast desktop CPUs, but may be slow on Raspberry PIs or cheap VMs exhausted their burst quota.
There was a problem hiding this comment.
Yeah, I've only discovered Lazy when I was fixing the test suite last night. Super cool and makes a lot of sense. Fixed.
Sure thing, fixed. |
This eliminates the newly added
edgedb-cli-mdlocal crate. It alsoallows to disable styling if stdout isn't tty and makes formatting more
flexible overall.