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

Merge & Diff Refactor #32

Merged
merged 11 commits into from
May 23, 2019
Merged

Merge & Diff Refactor #32

merged 11 commits into from
May 23, 2019

Conversation

rlizzo
Copy link
Member

@rlizzo rlizzo commented May 6, 2019

This PR introduces a custom implementation of the diff and merge algorithm which is designed to work with Hangar specifically. It's a bit rough around the edges, but seems to work well right now

This is a WIP and will be recieving rapid updates over the next few days.

@rlizzo rlizzo added enhancement New feature or request WIP Don't merge; Work in Progress labels May 6, 2019
@rlizzo rlizzo mentioned this pull request May 7, 2019
Closed
9 tasks
@hhsecond hhsecond mentioned this pull request May 7, 2019
Closed
@rlizzo rlizzo mentioned this pull request May 7, 2019
Merged
9 tasks
@rlizzo rlizzo requested a review from hhsecond May 7, 2019 16:35
@rlizzo
Copy link
Member Author

rlizzo commented May 7, 2019

@hhsecond can you please review this early state before I go through and do a cleanup and refactor for it's final form? It may not be the prettiest code yet, but the functionality should be complete right now.

@rlizzo rlizzo added this to the v0.1.0 Release milestone May 7, 2019
@rlizzo
Copy link
Member Author

rlizzo commented May 17, 2019

Hey @hhsecond can you give this a look? It still needs a bit of todying up to make it easier to understand on a reader, but for the most part the merging and conflict checking/resolution is fairly solid.

@rlizzo rlizzo added Awaiting Review Author has determined PR changes area nearly complete and ready for formal review. Bug: Priority 2 No risk of data/record corruption or loss; ANY user facing impacts and removed WIP Don't merge; Work in Progress labels May 23, 2019
hhsecond
hhsecond reviewed May 23, 2019
View reviewed changes
Copy link
Member

@hhsecond hhsecond left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me so far. I think we need more test cases for diff since we are exposing diff APIs through checkout object. I'll see if I could write some more (if you haven't already) and will finish the review along with that

src/hangar/checkout.py Outdated Show resolved Hide resolved
hhsecond
hhsecond reviewed May 23, 2019
View reviewed changes
Copy link
Member

@hhsecond hhsecond left a comment

Choose a reason for hiding this comment

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

Looks good to me so far. I think we need more test cases for diff since we are exposing diff APIs through checkout object. I'll see if I could write some more if you haven't already and will finish the review along with that

@rlizzo
Copy link
Member Author

rlizzo commented May 23, 2019

Looks good to me so far. I think we need more test cases for diff since we are exposing diff APIs through checkout object. I'll see if I could write some more if you haven't already and will finish the review along with that

Ok. please do write the test cases. I'm cleaning up the backend diff logic one last time to simplify it all. Anything user facing is good to go for you to work on!

The last think I'm going to add (user-facing) is a checkout.merge() method to write-enable checkouts so that the checkout can merge a branch directly. Otherwise this will be all set!

@rlizzo
Copy link
Member Author

rlizzo commented May 23, 2019 •
edited
Loading

Ok. This is ready for final review and testing!

Thanks for the input @hhsecond

@rlizzo rlizzo mentioned this pull request May 23, 2019
Closed
3 tasks
@rlizzo
Copy link
Member Author

rlizzo commented May 23, 2019

After an offline discussion with @hhsecond, we decided to merge this PR as it exists right now, and then add more test cases in a seperate PR before the 0.1 release. See #56 for test case progress

@rlizzo rlizzo merged commit 2c8143b into tensorwerk:master May 23, 2019
@rlizzo rlizzo deleted the diff-merge-refactor branch May 23, 2019 13:03
@rlizzo rlizzo mentioned this pull request May 23, 2019
Merged
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hhsecond hhsecond hhsecond left review comments

Assignees
No one assigned
Labels
Awaiting Review Author has determined PR changes area nearly complete and ready for formal review. Bug: Priority 2 No risk of data/record corruption or loss; ANY user facing impacts enhancement New feature or request
Projects
None yet
Milestone
v0.1.0 Release
Development

Successfully merging this pull request may close these issues.
2 participants
@rlizzo @hhsecond