Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API changes for v3 #21

Open
Morganamilo opened this issue Jun 5, 2021 · 8 comments
Open

API changes for v3 #21

Morganamilo opened this issue Jun 5, 2021 · 8 comments

Comments

@Morganamilo
Copy link
Member

Morganamilo commented Jun 5, 2021

  • Some getters should return Option Tracking for functions that should return Option #19
  • FileList::contains has unneeded Result<>
  • Alpm should not be Send
  • Rename CommitResult::FileConflict -> CommitResult::Fileconflicts
  • Rework Commit/Prepare Result to be lazy?
  • Add error and Commit/Prepare Result should be more consistent with eachother
  • Package::files() should be an option. No it's fine as is. Actually no
  • Remove handle needed to make an alpmlistmut and remove lifetimes.
  • &mut self for string setters
@fosskers

This comment has been minimized.

@Morganamilo

This comment has been minimized.

@Morganamilo

This comment has been minimized.

@fosskers

This comment has been minimized.

@Morganamilo
Copy link
Member Author

trans prepare (and commit) currently look like this:

pub fn trans_prepare(&mut self) -> Result<(), (PrepareResult<'_>, Error)>

With PrepareResult being:

pub enum PrepareResult<'a> {
    PkgInvalidArch(AlpmListMut<'a, Package<'a>>),
    UnsatisfiedDeps(AlpmListMut<'a, DependMissing>),
    ConflictingDeps(AlpmListMut<'a, OwnedConflict>),
    Ok,
}

This can be a bit awkward as you can't just handle.trans_prepare()? as PrepareResult doesn't implement Error or Display. It's also hard to pass with anyhow as due to storing the alpm lists the value is !Send and !Sync. It makes it a little awkward as you have to .map_err(|e| e.1) and you also lose some of the information.

Also PrepareResult should be named PrepareError.

It should probably be more like trans_add_pkg() which returns Result<(), AddError> . Store the error and the value and implement std Error and into alpm::Error. The function should also just hold a raw alpmlist and have a getter to turn it the error value.

@Morganamilo
Copy link
Member Author

I'm considering inverting how the structs work for v3.

instead of a Pkg struct that contains a pointer it will be &Pkg that contains an alpm_pkg_t. Then like &str, &Pkg will only be holdable by reference. I think that may work better for some ergonomics.

@Morganamilo
Copy link
Member Author

Unions can be done in a way that doesn't require us to match on them: https://github.com/rust-lang/rfcs/blob/master/text/2195-really-tagged-unions.md

@Morganamilo
Copy link
Member Author

I got around to implementing all of this in https://github.com/archlinux/alpm.rs/tree/v3.

Only think not done is transmuting the unions to enums directly. As due to padding that would require putting all the fields in the enum variant directly and not a struct. Which then causes issues as I wish to keep them private.

Don't expect this any time soon as I don't see a new pacman any time soon and it also depend on an un merged patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants