libgitdit is too hard to use

[matthiasbeyer]

I am currently in the process of writing an app with libgitdit and I found that it is way to hard to use.

For example, for checking whether an issue is "open", this code has to be written:

pub fn issue_is_open<'a>(i: &Issue<'a>) -> Result<bool> {
    use libgitdit::trailer::accumulation;
    use libgitdit::trailer::accumulation::Accumulator;
    use libgitdit::trailer::TrailerValue;
    use libgitdit::Message;

    let policy  = accumulation::AccumulationPolicy::Latest;
    let mut acc = accumulation::SingleAccumulator::new("Dit-status".to_owned(), policy);

    let mut messages = vec![];
    for message in i.messages()? {
        let mut trailers = message?.trailers().collect();
        messages.append(&mut trailers);
    }

    acc.process_all(messages.into_iter());

    if let Some((_, val)) = acc.into_iter().next() {
        match val {
            TrailerValue::String(s) => s == "OPEN" || s == "open" || s == "Open",
            _ => false,
        };
    }

    return Ok(false);
}

(The actual check for "open", "OPEN" and "Open" is trivial, but the aggregating of trailers is way too complex).

Of course, this functionality should be part of libgitdit as in Issue.get_latest_trailer("Dit-status")? == "Open" for example, but that's another problem.

The boilerplate for writing this code is way to much. This is only one example, there are other places where this library is too complex to use.

[neithernut]

I thought I had already introduced some functionality for handling accumulation in a more dev-friendly way. The passage where you collect trailers ended up being something like a simple fold in the binary IIRC. Nonetheless: yes, especially collection of trailers from accumulators might be a higher-level functionality which deserves an extra function or something. (e.g. put in a HEAD or range of messages as well as one or more accumulators and get the result). Could you elaborate about the other cases? Wouldn't it make more sense to open an issue for each use-case/interface?

[matthiasbeyer]

One thing which quickly comes to mind is Result<Iterator<Item = Result<_>>> which is returned by Issue::messages() afaicr. That is a cumbersome interface, but necessary. I guess I'll address the use case Result<Iterator<Item = Result<T>>> -> Result<Collection<T>> in my resiter crate.

[matthiasbeyer]

In general: Some high-level functionality (as you described) would be awesome.

What I can think about right now:

• Issue::latest_trailer_value(trailer_name: &str) -> Result<Option<_>>
• Issue::latest_trailer_value_from(trailer_name: &str, Oid) -> Result<Option<_>> where the user of the function can provide a "head" for the aggregation
• Functions on Issue which work on Issue::initial_message() like Issue::author(), Issue::committer(). Those are just convenience functions, but make the API less verbose.

What's missing, afaict:

• Issue::reply() and Message::reply() or similar functionality - basically a "leightweight" Issue::add_message(), which is rather complicate to use, IMO.

[neithernut]

Issue::latest_trailer_value(trailer_name: &str) -> Result<Option<_>>

Not possible because (a) the accumulation rule is not provided and (b) the choice of HEAD to use is outside the scope of the library (e.g. you generally need to specify a search order).

Issue::latest_trailer_value(trailer_name: &str) -> Result<Option<_>>

Still no accumulation rule

Functions on Issue which work on Issue::initial_message() like Issue::author(), Issue::committer(). Those are just convenience functions, but make the API less verbose.

In fact, I had considered those. However, we have to check whether it would violate our self-imposed dependency constraints.

Issue::reply() and Message::reply() or similar functionality - basically a "leightweight" Issue::add_message(), which is rather complicate to use, IMO.

I fear those are also not possible: we need a ref to a repo for one thing, and most of the parameters are not optional, either.

[matthiasbeyer]

Issue::latest_trailer_value_from(trailer_name: &str, Oid) -> Result<Option<_>>

no accumulation rule

What do you mean? You want the "latest" or "newest" value of the trailer at trailer_name? Where's the problem?

Issue::reply() / Message::reply()

I fear those are also not possible: we need a ref to a repo for one thing, and most of the parameters are not optional, either.

We could pass them: Issue::reply(author, committer, message) and Message::reply(author, committer, message, repo).

• tree defaults to empty tree here
• parents is implicitely given by the object beeing called on.

In the case of Issue we have a reference to the Repository and in the case of Message it could be passed.

Of course, these are just convenience functions, for the "trivial case". I'm not trying to replace Issue::add_message() with those, just making the interface simpler to use in the trivial case. I would even remove the difference between author and committer for this (so that the user has only to provide one Signature), to make it as simple as possible to use.

[neithernut]

no accumulation rule What do you mean? You want the "latest" or "newest" value of the trailer at trailer_name? Where's the problem?

Nope, sometimes we want a list of values. Consider, for example, Signed-off-by. And in the future we could also allow more complex values.

We could pass them: Issue::reply(author, committer, message) and Message::reply(author, committer, message, repo).

Issue::reply looks reasonable, but I'm not exactly a fan of passing the repo (which is essentially a context), especially as last parameter. However, having only one of both reply functions would be annoying.