Returns an Option for optional attributes of a package

[0x1793d1]

Linked to #3.

Only applied on package level. libalpm only ensures that pkgname and pkgver are defined.

[Morganamilo]

Don't think I like this. I didn't consider fields like isize and build date may not be defined. Seems as makepkg will always fill them in. But I guess if ALPM accepts a package without this data then it's valid.

In that case I may prefer these functions to unwrap by default and have try_isize() return an option.

Also from_cstr_optional can be written as:

s.as_ref().map(|s| CStr::from_ptr(s).to_str().unwrap())

[Morganamilo]

By the way, I have a dummy package with an install size of 0. This PR would make the install size show up as None instead of Some(0), which is incorrect. There's no real way for us to tell which one it should. That's just a limitation of pacman.

[0x1793d1]

OK thanks for your answer.

[0x1793d1]

For the build date, I just copied the same behavior as the install date. I saw that the install size can be of -1 if the value is incorrect (e.g. not a number).

[Morganamilo]

Patch looks good to me right now. I'll have to read through some more of ALPM first just to sanity check what it does for missing options like base64_sig and the numerical values.

[Morganamilo]

In the original issue I said:

the function alpm_pkg_get_desc() actually returns an empty string, not a null pointer.

Well from my testing, I was half right.

My dummy pkgbuild generates an empty desc:

# Generated by makepkg 5.2.1
# using fakeroot version 1.24
pkgname = test
pkgbase = test
pkgver = 1-1
pkgdesc = 
url = 
builddate = 1579109257
packager = Unknown Packager
size = 0
arch = x86_64

While your incomplete package generates:

pkgname = pacman
pkgver = 5.1.3-1

In my package, alpm creates a package where desc is an empty string, in yours the desc is null. This means for my package desc will be Some(""), and yours will be None.

I guess this does match how alpm does it. But it does seem like more hassle than it's worth, seems as 99% of packages are generated by makepkg.

So now i'm not really sure what to do.

We could accept this patch to mimic alpm. But then you'd have to still deal with an empty string and do something like:

desk.filter(|s| !s.is_empty()).unwrap_or("None")

Keeping it how it is now, you still have to deal with the empty string.

Or we could make the getters return None if the string is empty.

I'm leaning towards the last option seems as in alpm there's no way to tell the lack of a desc and an empty desc apart. (pkgdesc="" generates the same metadata as if you left it out).

[0x1793d1]

You are right, if we want to mimic alpm, we could let the patch as is. But the interest of the modification will be really limited. If we want to be closer to the pacman logic, we need to return None if the string is empty too.

E.g. when pacman displays string fields, it considers NULL and "" as None.

void string_display(const char *title, const char *string, unsigned short cols)
{
    if(title) {
        printf("%s%s%s ", config->colstr.title, title, config->colstr.nocolor);
    }
    if(string == NULL || string[0] == '\0') {
        printf(_("None"));
    } else {
        /* compute the length of title + a space */
        size_t len = string_length(title) + 1;
        indentprint(string, (unsigned short)len, cols);
    }
    printf("\n");
}

The initial reason of my PR was to be able to make the distinction between filled fields and those that are not in a more elegant way than doing if field.is_empty() { ... } else { ... }, especially if a package is modified by hand.

[Morganamilo]

Then if you've in agreement, lets do the third way.

s.as_ref().filter(|s| !s.is_empty).map(|s| CStr::from_ptr(s).to_str().unwrap())

Should work. just an extra .filter() added to the existing function.

[Morganamilo]

Also when you're done. Can you please rebase the commits together into one? I'll merge them right after that.

[0x1793d1]

Done.

[Morganamilo]

Looks good now, thanks.