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.

Sign up for GitHub

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

Jump to bottom

refactor indexes #40

Merged
merged 5 commits into from Apr 12, 2020
Merged

refactor indexes #40

merged 5 commits into from Apr 12, 2020

Conversation

danieleades
Copy link
Contributor

@danieleades danieleades commented Mar 21, 2020 •
edited

closes #41

@danieleades
Copy link
Contributor Author

there's a lot of scope for refactoring here. It's a tad uncomfortable doing it without any tests (see #37)

@danieleades
refactor the 'cli' index
cda9d20 
split the concerns of managing the filesystem and managing the git repository into separate objects
@danieleades
move the index::Tree into a separate module
56b339b
@danieleades
refactor index::git2 to use 'Tree'
681b10d
@danieleades danieleades changed the title crate metadata refactoring refactor indexes Mar 23, 2020
@Hirevo Hirevo mentioned this pull request Mar 24, 2020
Closed
@Hirevo Hirevo added the C-refactor Category: Refactor label Mar 24, 2020
@danieleades danieleades marked this pull request as ready for review March 24, 2020 20:05
@danieleades
Copy link
Contributor Author

i think it might also make sense to move the enum further down the chain, so the top level Index contains a Tree and a Repository. That Repository could be an enum with the two git implementations.
That way the Tree only appears once.
Does that make sense?

Hirevo
Hirevo approved these changes Apr 11, 2020
View reviewed changes
Copy link
Owner

@Hirevo Hirevo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my too long absence.
The refactor, as it is now, is already convincing, I am all for it and I didn't find anything to contest here.
About the duplication of the tree: Tree field in each Index, I agree that we can indeed move it to a Repository struct, that would associate an Index with a Tree.
So, how do you want to go about this ?
Do we do it inside of this PR or we merge this first ?

@danieleades
Copy link
Contributor Author

I think this is enough changes for one commit.

I would then open a new pull request to move the enum downwards from the Index to a Repository

@Hirevo Hirevo merged commit 59abbfe into Hirevo:master Apr 12, 2020
@danieleades danieleades deleted the metadata-refactor branch April 12, 2020 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hirevo Hirevo Hirevo approved these changes

Assignees
No one assigned
Labels
C-refactor Category: Refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

code duplication between index implementations
2 participants
@danieleades @Hirevo