Making configuration more flexible #457

Michael-F-Bryan · 2017-09-30T12:52:26Z

This attempts to start cleaning up the configuration API. The idea is that all items should be public seeing as a Config struct is just a bag of options with no behaviour (besides convenience methods for loading from disk and such).

Removing the getters and setters and builder pattern may be a bit controversial, but my reasoning is you usually use getters/setters to allow for forward compatibility without breaking downstream. However, if we remove a config item that someone downstream is using, I'd want their build to break. This indicates they are using an option which is no longer available and they should update their stuff accordingly.

I'm also trying to make sure the html renderer isn't special. You still specify html renderer-specific options using the output.html table, but the output table itself is just a BTreeMap<String, toml::Value>. To make life more convenient for downstream I've added a try_get_output() method which allows you to get a table according to key and deserialize it automatically using serde.

This way, later on when we give the rendering and plugin pipeline a makeover, you can pass in a reference to the global Config struct and individual renderers/plugins can get the options specific to it.

Michael-F-Bryan · 2017-09-30T12:53:35Z

So far I've just created the type definitions and a couple tests to make sure it deserializes as I expect it to. I'm going to start migrating the existing mdbook across to use the new configs now.

Michael-F-Bryan · 2017-09-30T14:17:46Z

I finished updating everything which relies on configuration. Now that MDBook no longer has dozens of builder methods for updating the configuration, it should allow us to make the crate a lot less decoupled and stateful.

All the tests pass on my machine. @azerupi or @budziq, would you be able to review this?

Phaiax

Much better than the getter/setter config.

Phaiax · 2017-10-11T18:46:40Z

src/renderer/html_handlebars/hbs_renderer.rs

@@ -283,50 +295,52 @@ impl Renderer for HtmlHandlebars {

        let rendered = self.post_process(rendered,
                                         "print.html",
-                                         book.get_html_config().get_playpen_config());
+                                         &book.config.html_config().unwrap_or_default().playpen);


There is already a html_config in scope (l. 243)

Phaiax · 2017-10-11T18:50:01Z

src/renderer/html_handlebars/hbs_renderer.rs

        data.insert("livereload".to_owned(), json!(livereload));
    }

    // Add google analytics tag
-    if let Some(ref ga) = book.get_google_analytics_id() {
+    if let Some(ref ga) = book.config.html_config().and_then(|html| html.google_analytics) {


Why do you use book.config instead of the config argument? (I guess there is a purpose, but I haven't read the whole source yet.)

Good point! I think I was blindly trying to fix the compile errors and not thinking of the bigger picture, but this should really use the config argument.

That way, when we start restructuring the renderer/plugin interface you can pass in just the Book (representation of a book on disk) and the Config. Avoiding passing around the small kitchen sink which is MDBook 😜

Phaiax · 2017-10-11T18:53:35Z

tests/config.rs

-}
+// #[test]
+// fn do_not_overwrite_unspecified_config_values() {
+//     let dir = TempDir::new("mdbook").expect("Could not create a temp dir");


still relevant?

Phaiax · 2017-10-11T18:56:32Z

src/renderer/html_handlebars/hbs_renderer.rs


        debug!("[*]: Register handlebars template");
        handlebars.register_template_string("index", String::from_utf8(theme.index.clone())?)?;

        debug!("[*]: Register handlebars helpers");
        self.register_hbs_helpers(&mut handlebars);

-        let mut data = make_data(book)?;
+        let mut data = make_data(book, &book.config)?;


Why adding the additional argument?

Phaiax · 2017-10-11T19:08:03Z

src/config.rs

+    /// This is for compatibility only. It will be removed completely once the
+    /// rendering and plugin system is established.
+    pub fn html_config(&self) -> Option<HtmlConfig> {
+        self.try_get_output("html").ok()


This is not cached, so it will be deserialized a few times, but I think there is no need to optimize this.
And editing a variable in code is not as straight forward as with the BookConfig. Maybe you should add a line in the documentation on how to change a html config variable.

Hmm... To be honest, I hadn't thought about letting people mutate the configuration after it has been loaded.

I think I'll rewrite the config to internally hold the raw Value, with getters and setters for accessing things like an output table (e.g. HtmlConfig) by key. The toml crate has a get_mut() method which I could expose to people so they can get something by index (e.g. output.html) and possibly mutate it.

I just experimented with Config holding a raw Value and then providing getters and setters for the various components (book settings, outputs, etc) and it doesn't end up being overly ergonomic. The issue is that every time you want to access something from the BookConfig you need to deserialize it again, which requires dealing with errors and can't really be cached.

I think if I leave the internals pub that should be enough to let people update and inspect the config if they need to.

I also tried a little but, but it did not end well, (see comment in line 57).
https://play.rust-lang.org/?gist=29a32500c428bfae20593421ec7bc4ab&version=stable

I would propose two other ways:

Do not use toml::Value at all and just use HtmlConfig as the only struct in output with direct deserialization. Add an API to get the original config string (raw file contents), so that the plugin can deserialize for itself.

Move the HtmlConfig struct into the HtmlHandlebars. Let the HtmlHandlebars deserialize from a toml::Value and store it's own deserialized config.

In the long run I want to go for number 2 because that'll completely separate the handlebars renderer from the book. I think I marked the HtmlConfig getter as deprecated to indicate that.

Phaiax · 2017-10-11T19:09:20Z

src/config.rs

+            .try_into()
+            .chain_err(|| "Couldn't deserialize the value"),
+        None => bail!("Key Not Found"),
+    }


Can these two match arms produce a more helpful error message?
It is only helpful for library users, but still.

Yeah good point. Although the deserializing error is chained, meaning if you iterate through the causes you can see the root reason for why deserializing failed. Giving you a high level "deserializing failed" message, plus a more low level error if you need more information.

I'm not sure what to do in the None case though. I really want to avoid returning an Option<Result<T>> if I can, that just feels ugly.

Phaiax · 2017-10-11T19:22:18Z

I almost wanted to ask if I can refactor the config, but then I found this nice PR.

I did a simple review and I vote for merging this.

Michael-F-Bryan · 2017-10-16T12:48:01Z

I've just rebased and fixed up the things @Phaiax raised. @azerupi, do you reckon this is ready to merge?

azerupi · 2017-10-18T12:15:29Z

src/bin/build.rs

-            "--curly-quotes 'Convert straight quotes to curly quotes, except for those \
-             that occur in code blocks and code spans'",
-        )
-        .arg_from_usage(
            "[dir] 'A directory for your book{n}(Defaults to Current Directory \


Why is the "curly quote" option removed?

I think I removed it because something like that belongs more in the config file than as a command line argument.

My thoughts were that options to alter the generated output and which usually stay the same belong in the config file, while options that alter runtime behaviour (e.g. --open or --no-create) are command line arguments... Does that sound logical?

That sounds reasonable to me, but I am not sure about the intent behind the curly quote option.

Phaiax · 2017-10-23T11:04:36Z

ping @azerupi

Michael-F-Bryan · 2017-11-11T06:02:45Z

I'm going to go back and do another review of this PR to check that it works as we want. @Phaiax, I know you need access to configuration as part of #51, so when I'm happy would you like to give this another look over and see if it suits your needs?

At the moment I'm thinking of giving people access to the book.toml config file's contents with some config.get("search") method that returns an arbitrary toml::Value which will be specific to their plugin/feature/use case. You can also convert a Value into any arbitrary serializable thing using Value::try_into().

Michael-F-Bryan · 2017-11-11T18:05:16Z

I think I've found quite a nice API for the Config. It's probably easiest to explain by showing a couple of the tests:

#[test]
fn load_arbitrary_output_type() {
    #[derive(Debug, Deserialize, PartialEq)]
    struct RandomOutput {
        foo: u32,
        bar: String,
        baz: Vec<bool>,
    }

    let src = r#"
    [output.random]
    foo = 5
    bar = "Hello World"
    baz = [true, true, false]
    "#;

    let should_be = RandomOutput {
        foo: 5,
        bar: String::from("Hello World"),
        baz: vec![true, true, false],
    };

    let cfg = Config::from_str(src).unwrap();
    let got: RandomOutput = cfg.get_deserialized("output.random").unwrap();

    assert_eq!(got, should_be);

    let baz: Vec<bool> = cfg.get_deserialized("output.random.baz").unwrap();
    let baz_should_be = vec![true, true, false];

    assert_eq!(baz, baz_should_be);
}

(get_deserialized() is just a helper which will get() a key then use serde to deserialize it automagically)

And I've also added a get_mut() function so you can mutate elements in the Config.

#[test]
fn mutate_some_stuff() {
    // really this is just a sanity check to make sure the borrow checker
    // is happy...
    let src = COMPLEX_CONFIG;
    let mut config = Config::from_str(src).unwrap();
    let key = "output.html.playpen.editable";

    assert_eq!(config.get(key).unwrap(), &Value::Boolean(true));
    *config.get_mut(key).unwrap() = Value::Boolean(false);
    assert_eq!(config.get(key).unwrap(), &Value::Boolean(false));
}

@budziq if you get a moment, can you have a look at this and let me know what you think? All the interesting stuff is in src/config.rs.

Phaiax · 2017-11-11T21:14:05Z

This is a good design. 👍

I only looked over the config.rs and that seems good to me.

Michael-F-Bryan · 2017-11-12T09:55:05Z

Something that's just occurred to me is that this will be a breaking change. At the moment configuration things specific to the book are present as top level items, e.g.

title = "my book"
authors = ["Michael-F-Bryan"]

[output.html]
theme = "./path/to/theme"

Whereas with this configuration I've taken some inspiration from Cargo.toml and put the "metadata" stuff under their own section.

Meaning the previous example should now look like this:

[book]     #   <-- new section
title = "my book"
authors = ["Michael-F-Bryan"]

[output.html]
theme = "./path/to/theme"

We're not 1.0 so technically we can do whatever we want, but at the same time I'd prefer not to break people's stuff without letting them know how to fix it. I think I'll add a compatibility thing to Config's Deserialize impl which will check for the previous format and load that, emitting a warning to tell the user to upgrade (with a link to the configuration page in the docs).

Phaiax

ill restart the review because a new commit arrived

Phaiax · 2017-11-12T12:43:25Z

book-example/src/format/config.md

-
-source = "my-src"  # the source files will be found in `root/my-src` instead of `root/src`
+src = "my-src"  # the source files will be found in `root/my-src` instead of `root/src`
+build-dir = "build"


You moved the build-dir into the common configuration. Hmmm. If I have multiple output generators, I cannot put them into different directorys. I think the other way was better. What are the arguments for doing it this way?

My thoughts were that (in the long term) you would have a single build/ directory where all renderers would put their rendered output. During the rendering stage a particular renderer is simply told the directory they should put things in, instead of each renderer doing their own thing and deciding it themselves.

That way mdbook would ensure you have a standardized directory layout looking something like this:

my_book

build

html

pdf

some_other_renderer

src

book.toml

With the special case being that if you're only using one renderer, mdbook will put tell a renderer to put its stuff directly in the root (avoiding the redundant extra level of nesting from build/html/).

ok, that sounds fine :)

Phaiax · 2017-11-12T12:47:43Z

src/bin/build.rs

-        Some(dest_dir) => book.with_destination(dest_dir),
-        None => book,
-    };
+    if let Some(dest_dir) = args.value_of("dest-dir") {


.. did you move the destination because this argument must be independent of the backend?

Yeah. I was thinking a renderer would be told to put its stuff in the $dest-dir/renderer_name/ directory, or $dest-dir/ if there's only one renderer.

That way renderers are "sandboxed" and should only be touching stuff in the directory they're told to write output to. #409 means renderers are being given an entirely in-memory representation of the book, so these two combined would mean we're a lot more decoupled from the file system and the environment.

Phaiax · 2017-11-12T13:07:04Z

src/config.rs

+        }
+
+        cfg.rest = table;
+        cfg


Phaiax · 2017-11-12T13:13:24Z

src/config.rs

+        if is_legacy_format(&table) {
+            warn!("It looks like you are using the legacy book.toml format.");
+            warn!("We'll parse it for now, but you should probably convert to the new format.");
+            warn!("See the mdbook documentation for more details");


The explicit differences are mentioned neither here nor in the docs. I think it would be a nice gesture to just say:

In short words: Just move all top level configuration entries like title, author and description into a group called [book].

Good point. I've seen rustc and other tools emit messages along the same line of "this is going to stop working soon so update your stuff. If you do x, y, and z then that should fix 99% of your problems" and found it a really nice gesture.

Phaiax · 2017-11-12T13:13:57Z

src/config.rs

+            warn!("It looks like you are using the legacy book.toml format.");
+            warn!("We'll parse it for now, but you should probably convert to the new format.");
+            warn!("See the mdbook documentation for more details");
+            warn!("http://azerupi.github.io/mdBook/format/config.html");


Just moved into rust-lang-nursery.

Just updated the link and it seems to work.

Phaiax · 2017-11-12T13:28:20Z

I vote for merging this finally. 💃
Any bugs or changes can be fixed later, we don't release immediately anyway.

Michael-F-Bryan · 2017-11-12T13:32:29Z

Any bugs or changes can be fixed later, we don't release immediately anyway.

Awesome! I'll wait for travis to give the green light before hitting the merge button.

steveklabnik · 2017-12-08T19:45:43Z

So, this is now causing the nomicon to fail building: https://travis-ci.org/rust-lang-nursery/nomicon/builds/313641706?utm_source=email&utm_medium=notification

is book.toml required now?

Michael-F-Bryan · 2017-12-09T09:42:56Z

It looks like that was an accidental regression, when loading the config we should fall back to Config::default() if book.toml doesn't exist. I've made a PR (#506) to fix this.

Making configuration more flexible

Michael-F-Bryan changed the title ~~Config~~ Making configuration less coupled and more extensible Sep 30, 2017

Michael-F-Bryan changed the title ~~Making configuration less coupled and more extensible~~ Making configuration more flexible Sep 30, 2017

Michael-F-Bryan mentioned this pull request Oct 1, 2017

Spaghetti Time! #458

Open

6 tasks

azerupi self-requested a review October 1, 2017 11:17

Michael-F-Bryan force-pushed the config branch from 64f3d2c to 2699dfd Compare October 4, 2017 11:56

Phaiax reviewed Oct 11, 2017

View reviewed changes

Michael-F-Bryan force-pushed the config branch from 2699dfd to d752257 Compare October 16, 2017 12:47

azerupi reviewed Oct 18, 2017

View reviewed changes

Phaiax mentioned this pull request Oct 22, 2017

Search with Elasticlunr #472

Closed

6 tasks

Phaiax mentioned this pull request Nov 8, 2017

Have a search bar #51

Closed

Phaiax reviewed Nov 12, 2017

View reviewed changes

Michael-F-Bryan added 7 commits November 12, 2017 21:37

Added some basic configuration objects

6059883

Added some basic deserializing tests and helpers

1d22a9a

Changed to the new config types

8d7970b

Removed old configs from MDBook

c056b5c

Removed all references to old the configuration from the html renderer

b74c2c1

Upgraded binaries to new configuration API

ddb0834

main library tests pass

cee3296

Michael-F-Bryan added 9 commits November 12, 2017 21:37

Removed the now redundant config files

1743f2a

Integration tests pass again

18c725e

All tests pass again :)

1b5137c

Rebased after rust-lang#438

d37821c

Added in things from @Phaiax's review

3aa6436

Went back and simplified Config to be a smart wrapper around toml::Table

c25c5d7

Updated the configuration chapter in the book-example

c777913

Added in legacy config format support

238dfb7

Updated the warning to give some basic migration instructions

5eff572

Michael-F-Bryan force-pushed the config branch from c2ca154 to 5eff572 Compare November 12, 2017 13:37

Michael-F-Bryan merged commit fb99276 into rust-lang:master Nov 12, 2017

This was referenced Nov 12, 2017

Fix copy to clipboard is available only for rust snippets #447

Merged

update the documentation for 0.1.0 release #368

Closed

Michael-F-Bryan deleted the config branch November 18, 2017 11:06

Michael-F-Bryan mentioned this pull request Dec 9, 2017

Added a quick fix so if the config isn't found we use a default #506

Merged

This was referenced Dec 30, 2017

Project Maintenance Discussion #482

Closed

[Discussion] Book representation #146

Closed

[Discussion] Change configuration file to toml instead of json #96

Closed

Michael-F-Bryan mentioned this pull request Jan 19, 2018

0.1 Release Prerequisites #558

Closed

3 tasks

Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020

Merge pull request rust-lang#457 from Michael-F-Bryan/config

7e50761

Making configuration more flexible

Making configuration more flexible #457

Making configuration more flexible #457

Conversation

Michael-F-Bryan commented Sep 30, 2017

Michael-F-Bryan commented Sep 30, 2017

Michael-F-Bryan commented Sep 30, 2017 • edited Loading

Phaiax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phaiax Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phaiax commented Oct 11, 2017

Michael-F-Bryan commented Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Michael-F-Bryan Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phaiax commented Oct 23, 2017

Michael-F-Bryan commented Nov 11, 2017

Michael-F-Bryan commented Nov 11, 2017 • edited Loading

Phaiax commented Nov 11, 2017

Michael-F-Bryan commented Nov 12, 2017

Phaiax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phaiax commented Nov 12, 2017

Michael-F-Bryan commented Nov 12, 2017

steveklabnik commented Dec 8, 2017

Michael-F-Bryan commented Dec 9, 2017

Michael-F-Bryan commented Sep 30, 2017 •

edited

Loading

Phaiax Oct 11, 2017 •

edited

Loading

Michael-F-Bryan Oct 16, 2017 •

edited

Loading

Michael-F-Bryan commented Oct 16, 2017 •

edited

Loading

Michael-F-Bryan Oct 19, 2017 •

edited

Loading

Michael-F-Bryan commented Nov 11, 2017 •

edited

Loading

Michael-F-Bryan Nov 12, 2017 •

edited

Loading