Add pretty-print functionality

[relausen-tlg]

With the changes in this PR, users can now provide various options for formatting output: Spaces around = or not, the number of blank lines between sections, and the number of spaces used to indent multi-line values.

I have not added tests for the new functionality yet, since I would like to get feedback on the ideas and principles used before I proceed.

[QEDK]

Looks good! Can you rebase this to main, and is there any reason we cannot use derive-builder on the Ini struct? Seems like a missed opportunity. But also, derive-builder should actually be called on the internal map, and not the struct itself - unless we can do something like, ini.map.section = HashMap::new()

[relausen-tlg]

Thanks! I'm not sure I understand the need for rebasing - I believe the branch is up date with the latest changes on main, as per commit 6782f0d. That said, I did use a merge commit to update instead of rebasing :-)

WRT using derive-builder in the Ini struct: No, there is no reason not to add it there :-). It would make the "user experience" consistent, and that is always a good thing. We could also add to the IniDefault struct, I guess?

I have looked into this, and it is indeed possible to initialize collections with derive-builder. I have tested it locally, and it works fine, as far as I can see.

I changed the Ini struct to this:

#[derive(Debug, Clone, Eq, PartialEq)]
#[cfg_attr(feature = "derive_builder", derive(Builder), builder(default))]
#[non_exhaustive]
pub struct Ini {
    #[cfg_attr(feature = "derive_builder", builder(setter(each(name = "set_value"))))]
    map: Map<String, Map<String, Option<String>>>,
    default_section: std::string::String,
    comment_symbols: Vec<char>,
    delimiters: Vec<char>,
    boolean_values: HashMap<bool, Vec<String>>,
    case_sensitive: bool,
    multiline: bool,
}

impl Default for Ini {
    fn default() -> Self {
        let defaults = IniDefault::default();
        Self {
            map: Map::new(),
            default_section: defaults.default_section,
            comment_symbols: defaults.comment_symbols,
            delimiters: defaults.delimiters,
            boolean_values: defaults.boolean_values,
            case_sensitive: defaults.case_sensitive,
            multiline: defaults.multiline,
        }
    }
}

After this I can now use the builder pattern to build an Ini, only setting the map itself and having sensible default values for the other properties. This does require that Map is re-exported:

    let my_config = IniBuilder::default()
        .set_value(
            ("section1".into(),
                Map::from(
                    [
                        ("key1".into(), Some("value1".into())),
                        ("key2".into(), Some("value2".into())),
                        ("key3".into(), Some("value3".into())),
                    ]
                )
            )
        )
        .set_value(
            ("section2".into(),
                Map::from(
                    [
                        ("key4".into(), Some("value4".into())),
                        ("key5".into(), Some("value5".into())),
                        ("key6".into(), Some("value6".into())),
                    ]
                )
            )
        )
        .build()?;
    let ini_string = my_config.writes();

If I don't want to initialize the map, but, e.g., just want to enable multiline support, I can just write

    let my_config2 = IniBuilder::default()
        .multiline(true)
        .build()?;

Maybe we could make it a bit easier to initialize the map with a macro.

Should we go this way?

[QEDK]

Thanks! I'm not sure I understand the need for rebasing - I believe the branch is up date with the latest changes on main, as per commit 6782f0d. That said, I did use a merge commit to update instead of rebasing :-)

WRT using derive-builder in the Ini struct: No, there is no reason not to add it there :-). It would make the "user experience" consistent, and that is always a good thing. We could also add to the IniDefault struct, I guess?

I have looked into this, and it is indeed possible to initialize collections with derive-builder. I have tested it locally, and it works fine, as far as I can see.

I changed the Ini struct to this:

#[derive(Debug, Clone, Eq, PartialEq)]
#[cfg_attr(feature = "derive_builder", derive(Builder), builder(default))]
#[non_exhaustive]
pub struct Ini {
    #[cfg_attr(feature = "derive_builder", builder(setter(each(name = "set_value"))))]
    map: Map<String, Map<String, Option<String>>>,
    default_section: std::string::String,
    comment_symbols: Vec<char>,
    delimiters: Vec<char>,
    boolean_values: HashMap<bool, Vec<String>>,
    case_sensitive: bool,
    multiline: bool,
}

impl Default for Ini {
    fn default() -> Self {
        let defaults = IniDefault::default();
        Self {
            map: Map::new(),
            default_section: defaults.default_section,
            comment_symbols: defaults.comment_symbols,
            delimiters: defaults.delimiters,
            boolean_values: defaults.boolean_values,
            case_sensitive: defaults.case_sensitive,
            multiline: defaults.multiline,
        }
    }
}

After this I can now use the builder pattern to build an Ini, only setting the map itself and having sensible default values for the other properties. This does require that Map is re-exported:

    let my_config = IniBuilder::default()
        .set_value(
            ("section1".into(),
                Map::from(
                    [
                        ("key1".into(), Some("value1".into())),
                        ("key2".into(), Some("value2".into())),
                        ("key3".into(), Some("value3".into())),
                    ]
                )
            )
        )
        .set_value(
            ("section2".into(),
                Map::from(
                    [
                        ("key4".into(), Some("value4".into())),
                        ("key5".into(), Some("value5".into())),
                        ("key6".into(), Some("value6".into())),
                    ]
                )
            )
        )
        .build()?;
    let ini_string = my_config.writes();

If I don't want to initialize the map, but, e.g., just want to enable multiline support, I can just write

    let my_config2 = IniBuilder::default()
        .multiline(true)
        .build()?;

Maybe we could make it a bit easier to initialize the map with a macro.

Should we go this way?

I think this is more complicated than how it should be, I assume we could have an easy way to extend serde support but not really (from this angle). Here are my present thoughts:

• Let's remove the derive-builder dependency, it is not inherently helpful for pretty-writing (albeit from this approach it is)
• Let's make WriteOptions public, non-exhaustive (and always available) and part of pretty_write and pretty_writes (as well as the async variants)

Let's tackle derive-builder usage in a separate PR, I think pretty_write is more a use-case than everything else here.

[relausen-tlg]

I have been thinking more or less the same after I posted the example. I agree 100% with your thoughts. I will make your suggested changes tonight (CET, don't know which timezone you're in 😄 ).

[relausen-tlg]

I pushed changes to reflect what we agreed upon. Also added some tests and updated the README and the docs in lib.rs to include an example of pretty_write usage. Don't hesitate to let me know if you disagree with something or think something is missing!

[QEDK]

I made some slight changes to your implementation, mostly to help with creating a new WriteOptions, can you give your feedback at #45 and then we can merge it in.

[relausen-tlg]

Looks good to me - even though the new_with_params function demonstrates why the builder pattern is so great, given the lack of named parameters in Rust 😉 . I'm a happy camper, so let's ship it 😄

[relausen-tlg]

I take it that you handle bumping the version number to what you see fit?

[QEDK]

I take it that you handle bumping the version number to what you see fit?

Yeah, will handle that in a separate PR.