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

use a boxed trait for the Repository #52

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danieleades
Copy link
Contributor

depends on #50

this is what it would look like if the index enum was eliminated all-together (i've replaced it with a boxed trait).

The advantage of this is you can use any object capable of managing a git repo, not just the implementations defined in this crate.

i'd be interested to get your thoughts on this @Hirevo?

there's another alternative, which is to make the Index generic over the repository type, rather than boxing the trait. The calling code would then probably need to box the Index to avoid specifying a concrete type.

@danieleades danieleades changed the title Repository trait use a boxed trait for the Repository Apr 15, 2020
@danieleades
Copy link
Contributor Author

this should be rebased once(/if) #50 is merged

@danieleades
Copy link
Contributor Author

under this scheme, both the CLI and GIT2 strategies should be feature-gated (though can be included by default)

@codecov
Copy link

codecov bot commented Apr 15, 2020

Codecov Report

Merging #52 into master will increase coverage by 0.25%.
The diff coverage is 7.04%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #52      +/-   ##
=========================================
+ Coverage    0.00%   0.25%   +0.25%     
=========================================
  Files          50      47       -3     
  Lines        1988    1963      -25     
=========================================
+ Hits            0       5       +5     
+ Misses       1988    1958      -30     
Impacted Files Coverage Δ
src/config/mod.rs 0.00% <0.00%> (ø)
src/index/repository/cli.rs 0.00% <0.00%> (ø)
src/index/repository/git2.rs 0.00% <0.00%> (ø)
src/index/mod.rs 13.51% <19.23%> (+13.51%) ⬆️
src/db/mod.rs 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff77d0c...740df21. Read the comment docs.

@Hirevo
Copy link
Owner

Hirevo commented Apr 25, 2020

Initially, I would have said that there isn't much to be gained from using boxed dynamic dispatch vs an enum, but with the upcoming library-split of Alexandrie, I think dynamic dispatch makes more sense.
People could then implement their own index management crate and easily get Alexandrie to use it, if they want to.
Also, I guess that with what you pointed out in #51 (about mutexing the entire State), the type would be reduced to just Box<dyn Repository + 'static> (we would no longer require Send + Sync and allow for more diverse impls of Repository to be used in Index).
With all this, I would say that I am on board about doing this.

One thing, though, is that we may have the same kind of naming problem with Repo / Repository than with Index / Indexer.
If I remember correctly, an idea we had was to potentially rename Index / Indexer to LocalIndex / Index, so maybe we can do the same with LocalRepository / Repository ?

@Hirevo Hirevo added the C-refactor Category: Refactor label Apr 25, 2020
@danieleades
Copy link
Contributor Author

i'll take another look at the naming conventions. I agree with you there's some work to do there.

I cannot for the life of me remember in which order these pull requests should be merged (if they're merged)

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

Successfully merging this pull request may close these issues.

2 participants