Skip to content

tidy up merge status model and introduce rebase status model#7135

Merged
outofambit merged 7 commits intodevelopmentfrom
introduce-rebase-status-model
Mar 26, 2019
Merged

tidy up merge status model and introduce rebase status model#7135
outofambit merged 7 commits intodevelopmentfrom
introduce-rebase-status-model

Conversation

@shiftkey
Copy link
Member

Overview

Groundwork to complete #6960 and #6959

Description

This PR addresses two pieces of tech debt that I noticed while refreshing my memory on how we computed the "merge status" when comparing branches (either in the History sidebar or in the Merge Branch dialog).

  • We have a MergeResultKind which looks very similar to what I need for the rebase status
    • this has been renamed to ComputedActionKind and now lives in it's own module
  • We had two very similar models - MergeResultStatus in app state and MergeResult in the models folder
    • these have been unified under MergeResult so that we have a common model for this "merge status"

I also added in the RebasePreviewResult status which will be used in subsequent PRs to represent the "success"/"conflicts" rebase preview state at the start of the rebase flow.

Release notes

Notes: no-notes

@shiftkey shiftkey added tech-debt Issues and pull requests related to addressing technical debt or improving the codebase ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Mar 21, 2019
@shiftkey shiftkey added this to the 1.7.0 milestone Mar 21, 2019
@outofambit outofambit self-requested a review March 25, 2019 17:48
@shiftkey shiftkey force-pushed the introduce-rebase-status-model branch from 20c0077 to 29b8a39 Compare March 25, 2019 19:54
@shiftkey shiftkey force-pushed the introduce-rebase-status-model branch from 29b8a39 to 0e410ae Compare March 25, 2019 20:55
outofambit
outofambit previously approved these changes Mar 25, 2019
Copy link
Contributor

@outofambit outofambit left a comment

Choose a reason for hiding this comment

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

two (non-blocking) small things, but otherwise looks good. I like this level of abstraction!

@@ -0,0 +1,14 @@
/**
* A state representing the app computing whether a planned action will require
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I know the intent behind this enum by reading this but i'm not sure. could we add a couple examples to this comment of what "a planned action" is? (merges, rebases, etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me kick this down the road to the next PR - the rebase case is similar to merge but might help with clarifying these docs.

outofambit
outofambit previously approved these changes Mar 25, 2019
@shiftkey
Copy link
Member Author

Just pushed one last commit because I overlooked one interface -> type change which was caught by the lint step on CI...

@shiftkey shiftkey requested a review from outofambit March 26, 2019 12:31
@outofambit
Copy link
Contributor

I'm going to go ahead of another 24 hour waiting period and merge this since the changes since I approved yesterday are super minor.

@outofambit outofambit merged commit e447b17 into development Mar 26, 2019
@outofambit outofambit deleted the introduce-rebase-status-model branch March 26, 2019 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review Pull Requests that are ready to be reviewed by the maintainers tech-debt Issues and pull requests related to addressing technical debt or improving the codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants