rendering configuration from environment variables #291
Conversation
|
I apologise for the wait! |
There was a problem hiding this comment.
Overall I really like the idea of this feature! It makes the configuration system for frontend assets really flexible and gives the user a lot of control. I have a couple small nits, but overall the code looks well written! :)
We may also want to throw in some tests as a sanity check to make sure everything functions as intended and any changes down the track won't accidentally break things.
I've got a couple general questions on how this will interact with the wider configuration system though. A PR in the works gives the configuration system a large makeover and allows different renderers/frontends to have their own arbitrary settings. How would this environment variables way of configuring things interact with book.toml? And if the two both specify similar options, for example a themes/ directory which contains a copy of jquery and a MDBOOK_JQUERY_URL variable, which source for jquery gets precedence?
| /// returns true if this resource has to be embedded in the book | ||
| fn must_embed(&self) -> bool; | ||
| /// returns the url for this resource. This panics if must_render_url returns false. | ||
| fn url(&self) -> String; |
There was a problem hiding this comment.
Just out of curiosity, would there be any benefit to using a strongly typed Url here instead of a plain String?
| pub static HIGHLIGHT_URL: &'static str = &"highlight.js"; | ||
| pub static AWESOME_URL: &'static str = &"https://maxcdn.bootstrapcdn.com/font-awesome/4.3.0/css/font-awesome.min.css"; | ||
| pub static OPEN_SANS_URL: &'static str = &"https://fonts.googleapis.com/css?family=Open+Sans:300italic,400italic,600italic,700italic,800italic,400,300,600,700,800"; | ||
| pub static SOURCE_CODE_PRO_URL: &'static str = &"https://fonts.googleapis.com/css?family=Source+Code+Pro:500"; |
There was a problem hiding this comment.
I don't think any of these leading &'s are necessary. A string literal is usually a &'static str.
| /// returns the url for this resource. This panics if must_render_url returns false. | ||
| fn url(&self) -> String; | ||
| /// returns the source location for this resource, or None if none was configured. | ||
| /// This panics if must_embed returns false. |
There was a problem hiding this comment.
Isn't both panicking and returning an Option<_> here a bit of an antipattern? I would have thought a function which returns a fallible type should return None instead of panicking.
| } | ||
|
|
||
| /// parse font_awesome configuration from environment variables. | ||
| fn awesome() -> Awesome { |
There was a problem hiding this comment.
Wouldn't this be better as a function associated with Awesome (e.g. Awesome::new()) instead of just a normal free function?
| } | ||
|
|
||
| /// return a resource from various environment variables. | ||
| fn resource(resource: &str, url_default: &str) -> BasicResource { |
There was a problem hiding this comment.
This smells a lot like a constructor to me... Should it instead be moved to something like BasicResources::new()?
| /// Special struct for font-awesome, as it consists of multiple embedded files | ||
| /// but is configured as a directory. Methods implemented for this struct | ||
| /// calculate the paths of all the files in this directory. | ||
| pub struct Awesome { |
There was a problem hiding this comment.
This should probably also have a #[derive(Debug, Clone)] attribute.
| /// Configuration information for all the third-party resources. This specifies | ||
| /// for every resource whether it should be included at all, and if so, just as | ||
| /// an URL or also as an embedded resource. | ||
| pub struct Configuration { |
There was a problem hiding this comment.
Similar to Awesome and BasicResource, Configuration should also implement Debug. Clone is also a useful thing to have.
| } | ||
|
|
||
| /// The rendering strategy for a resource | ||
| enum Strategy { |
There was a problem hiding this comment.
This could probably derive Debug, Clone, Copy, and PartialEq (the last one makes testing nicer).
| } | ||
|
|
||
| /// Parse the configuration from the environment variables. | ||
| pub fn configuration_from_env() -> Configuration { |
There was a problem hiding this comment.
Can we add a couple integration tests (most probably in the tests/ directory) which set some environment variables and make sure Configuration_from_env() picks up the appropriate resources? The implementation itself is fairly straightforward, but it's always nice to have a couple sanity tests to make sure it all works and prevent any regressions from being introduced down the track.
| } | ||
|
|
||
| pub fn highlight_js(&self) -> Vec<u8> { | ||
| assert!(self.conf.highlight.must_embed()); |
There was a problem hiding this comment.
Just out of curiosity, what do we stand to gain from having these assert!() statements instead of returning something like a Result?
(continuing on my previous comment on the resource_content() method)
As discussed in #271, here's a patch that makes the mdBook output configurable through environment variables.
For details on the implemented scheme, see the comments under resources/.