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

Revision of the Indexer trait #60

Open
Hirevo opened this issue Apr 27, 2020 · 2 comments
Open

Revision of the Indexer trait #60

Hirevo opened this issue Apr 27, 2020 · 2 comments
Labels
C-refactor Category: Refactor help wanted Extra attention is needed

Comments

@Hirevo
Copy link
Owner

Hirevo commented Apr 27, 2020

The Indexer trait, in its current state has a number of problems that needlessly restricts what its implementors can do.

Here is a initial and non-exhaustive list of problems we might want to solve:

  1. Missing &mut self:
    This forces implementors to resort to interior mutability to workaround this constraint and it should be the responsibility of the registry itself to take care of the synchronization part (that it should be doing anyway, to avoid concurrent and racy crate publishes), so we should just allow them to take an &mut self and open the doors for fancier index managers.
  2. Error handling:
    The return types of the current trait are all just Result<_, Error> (_ being the type of successful value).
    So, in the case of fn latest_record(&self, name: &str) -> Result<CrateVersion, Error>, for instance, distinguishing between whether the crate was not found (something that can happen and not a real failure of the system) or something more critical is harder than needed (we must match and cannot just use ? to handle the critical ones).
    Taking inspiration of the error-handling design of Crate-Index, we could change some of these return type into Result<Option<_>, Error> or something like Result<Result<_, RetrievalError>, Error>.
    This way, the ? operator could take care of the outer, more critical, error and we can easily know if the lookup found anything or not.

Feel free to propose designs or share another problem that you think should be addressed by the new revision.

@Hirevo
Copy link
Owner Author

Hirevo commented Apr 27, 2020

Here is an initial draft for a revision (just attempting to address the points above, with no real semantic change to the API):

/// The required trait that any crate index management type must implement.
pub trait Indexer {
    /// Gives back the URL of the managed crate index.
    fn url(&self) -> Result<String, Error>;

    /// Refreshes the managed crate index (in case another instance made modification to it).
    fn refresh(&mut self) -> Result<(), Error>;

    /// Retrieves all the version records of a crate.
    fn all_records(&self, name: &str) -> Result<Option<Vec<CrateVersion>>, Error>;

    /// Retrieves the latest version record of a crate.
    fn latest_record(&self, name: &str) -> Result<Option<CrateVersion>, Error>;

    /// Retrieves the latest crate version record that matches the given name and version requirement.
    fn match_record(&self, name: &str, req: VersionReq) -> Result<Option<CrateVersion>, Error>;

    /// Commits and pushes changes upstream.
    fn commit_and_push(&self, msg: &str) -> Result<(), Error>;

    /// Adds a new crate record into the index.
    fn insert_record(&mut self, record: CrateVersion) -> Result<(), Error>;

    /// Alters an index's crate version record with the passed-in function.
    fn alter_record<F>(&mut self, name: &str, version: Version, func: F) -> Result<Option<CrateVersion>, Error>
    where
        F: FnOnce(&mut CrateVersion);

    /// Yanks a crate version.
    fn yank_record(&mut self, name: &str, version: Version) -> Result<Option<()>, Error>;

    /// Un-yanks a crate version.
    fn unyank_record(&mut self, name: &str, version: Version) -> Result<Option<()>, Error>;
}

@Hirevo Hirevo added the C-refactor Category: Refactor label Apr 27, 2020
@Hirevo
Copy link
Owner Author

Hirevo commented Apr 27, 2020

Another interesting question:
Currently, the trait is not object-safe due the generics used in fn alter_records(...).
Are we interested in making the new revised trait object-safe ?
(aka. do we intend to ever use a dyn Indexer ?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-refactor Category: Refactor help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant