Move from pass-by-value builder style API to pass-by-reference

[dralley]

The current style isn't very readable for RPMs of even average complexity, e.g.

 let built_rpm = PackageBuilder::new(
        "rpm-basic",
        "2.3.4",
        "MPL-2.0",
        "noarch",
        "A package for exercising basic features of RPM",
    )
    .epoch(1)
    .release("5")
    .description("This package attempts to exercise basic features of RPM packages.")
    .vendor("Los Pollos Hermanos")
    .url("http://www.savewalterwhite.com/")
    .vcs("https://github.com/rpm-rs/rpm")
    .group("Development/Tools")
    .packager("Walter White")
    .compression(CompressionType::None)
    .build_host("localhost")
    .source_date(1681068559)
    .provides(Dependency::any("/usr/bin/ls"))
    .provides(Dependency::any("aaronpaul"))
    .provides(Dependency::any("breaking(bad)"))
    .provides(Dependency::config("rpm-basic", "1:2.3.4-5.el9"))
    .provides(Dependency::eq("rpm-basic", "1:2.3.4-5.el9"))
    .provides(Dependency::eq("shock", "33"))
    .requires(Dependency::script_pre("/usr/sbin/ego"))
    .requires(Dependency::config("rpm-basic", "1:2.3.4-5.el9"))
    .requires(Dependency::greater_eq("methylamine", "1.0.0-1"))
    .requires(Dependency::less_eq("morality", "2"))
    .requires(Dependency::script_post("regret"))
    .conflicts(Dependency::greater("hank", "35"))
    .obsoletes(Dependency::less("gusfring", "32.1-0"))
    .obsoletes(Dependency::less("tucosalamanca", "444"))
    .supplements(Dependency::eq("comedy", "0:11.1-4"))
    .suggests(Dependency::any("chilipowder"))
    .enhances(Dependency::greater("purity", "9000"))
    .recommends(Dependency::any("SaulGoodman(CriminalLawyer)"))
    .recommends(Dependency::greater("huel", "9:11.0-0"))
    .with_file(
        "./tests/assets/SOURCES/example_config.toml",
        FileOptions::new("/etc/rpm-basic/example_config.toml").is_config(),
    )?
    .with_file(
        "./tests/assets/SOURCES/multiplication_tables.py",
        FileOptions::new("/usr/bin/rpm-basic"),
    )?
    .with_file(
        // I didn't bother filling all of these in yet, obviously it's not complete
        "",
        FileOptions::new("/usr/lib/rpm-basic").mode(FileMode::Dir { permissions: 0o644 }),
    )?
    .with_file(
        "",
        FileOptions::new("/usr/lib/rpm-basic/module").mode(FileMode::Dir { permissions: 0o755 }),
    )?
    .with_file(
        "./tests/assets/SOURCES/module/__init__.py",
        FileOptions::new("/usr/lib/rpm-basic/module/__init__.py"),
    )?
    .with_file(
        "./tests/assets/SOURCES/module/hello.py",
        FileOptions::new("/usr/lib/rpm-basic/module/hello.py"),
    )?
    .with_file(
        "",
        FileOptions::new("/usr/lib/rpm-basic/module").mode(FileMode::Dir { permissions: 0o755 }),
    )?
    .with_file(
        "",
        FileOptions::new("/usr/share/doc/rpm-basic").mode(FileMode::Regular { permissions: 0o644 }),
    )?
    .with_file(
        "",
        FileOptions::new("/usr/share/doc/rpm-basic/README").is_doc(),
    )?
    .with_file(
        "./tests/assets/SOURCES/example_data.xml",
        FileOptions::new("/usr/share/rpm-basic/example_data.xml"),
    )?
    .with_file(
        "",
        FileOptions::new("/var/log/rpm-basic/basic.log").is_ghost(),
    )?
    .with_file(
        "",
        FileOptions::new("/var/tmp/rpm-basic").mode(FileMode::Dir { permissions: 0o755 }),
    )?
    .add_changelog_entry(
        "Walter White <[email protected]> - 3.3.3-3",
        "- I'm not in the meth business. I'm in the empire business.",
        1623672000,
    )
    .add_changelog_entry(
        "Gustavo Fring <[email protected]> - 2.2.2-2",
        "- Never Make The Same Mistake Twice.",
        1619352000,
    )
    .add_changelog_entry(
        "Mike Ehrmantraut <[email protected]> - 1.1.1-1",
        "- Just because you shot Jesse James, don't make you Jesse James.",
        1619352000,
    )
    .build()?;

You can do minor things like adding line breaks, but that's not the only issue. Say you wanted to build an RPM from JSON, or user input or something at runtime, and you need to intertwine the builder with loops and if statements. This can be done but you would need to constantly re-assign the variable e.g. #193 (comment)

What we could potentially do instead, is switch to pass-by-reference style. e.g.

let mut builder = PackageBuilder::new(
        "rpm-basic",
        "2.3.4",
        "MPL-2.0",
        "noarch",
        "A package for exercising basic features of RPM",
    );

builder
    .epoch(1)
    .release("5")
    .description("This package attempts to exercise basic features of RPM packages.")
    .vendor("Los Pollos Hermanos")
    .url("http://www.savewalterwhite.com/")
    .group("Development/Tools")
    .packager("Walter White");

if github_repo {
    builder.vcs("https://github.com/rpm-rs/rpm");
}

builder
    .provides(Dependency::any("/usr/bin/ls"))
    .provides(Dependency::any("aaronpaul"))
    .provides(Dependency::any("breaking(bad)"))
    .provides(Dependency::config("rpm-basic", "1:2.3.4-5.el9"))
    .provides(Dependency::eq("rpm-basic", "1:2.3.4-5.el9"))
    .provides(Dependency::eq("shock", "33"));

for req in requirements {
    builder.requires(req);
}

That would simplify intertwining logic into the builder construction.

[juliusl]

I feel like this is what the fold/try_fold adapters are really good at.

For example, w/ pass-by-value that last example is just:

builder = requirements.fold(builder, |b, req| b.requires(req)); 

Similarly, for your other examples, it could be handled similarly,

// This can be created by reading in a file
let files = [
  ("./tests/assets/SOURCES/example_config.toml", FileOptions::new("/etc/rpm-basic/example_config.toml").is_config()),
  ("./tests/assets/SOURCES/multiplication_tables.py", FileOptions::new("/usr/bin/rpm-basic")),
  ("", FileOptions::new("/usr/lib/rpm-basic").mode(FileMode::Dir { permissions: 0o644 })),
  ("", FileOptions::new("/usr/lib/rpm-basic/module").mode(FileMode::Dir { permissions: 0o755 }))
];

builder = files.try_fold(builder, |b, (dest, opts)| {
  b.with_file(dest, opts)
})?;

[dralley]

For example, w/ pass-by-value that last example is just:

builder = requirements.fold(builder, |b, req| b.requires(req));

Well, you say "just" but that's a lot of boilerplate that ultimately does not really need to be there.

If there's no particular benefit from passing by ownership in this context, then why sacrifice usability to do so?

[juliusl]

For example, w/ pass-by-value that last example is just:

builder = requirements.fold(builder, |b, req| b.requires(req));

Well, you say "just" but that's a lot of boilerplate that ultimately does not really need to be there.

If there's no particular benefit from passing by ownership in this context, then why sacrifice usability to do so?

I think I see your point (I hope I didn't offend w/ my usage of just). Since it looks like in your example you're passing back &mut Self the existing builder pattern pretty much stays the same.

However, this last portion,

for req in requirements {
    builder.requires(req);
}

Seemed to conflict w/ the stated problem of and you need to intertwine the builder with loops and if statements.

I wanted to point out that since an iterator adapter exists, you could make this improvement today via an extension function that can reduce/fold state without needing to change the existing api.

And turn this into a 1 liner on the consumer side,

builder.requires(requirements);

[dralley]

I don't see how it's a conflict, exactly. I agree that you can do it via iterator adapter, but I'm not certain that it's the most ergonomic way.

Another potential reason, though, is that I'd kinda like to expose Python bindings eventually and I think simple reference-based APIs would make that easier.