-
-
Notifications
You must be signed in to change notification settings - Fork 150
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
#446 mob start --discard-uncommitted-changes #447
Conversation
|
||
Thank you @stefanscheidt for this feature request | ||
|
||
# 5.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, we released 5.2.0 on socratesAT, we need to remove this paragraph :)
mob.go
Outdated
@@ -527,7 +527,7 @@ func deleteRemoteWipBranch(configuration config.Configuration) { | |||
|
|||
func start(configuration config.Configuration) error { | |||
uncommittedChanges := hasUncommittedChanges() | |||
if uncommittedChanges && !configuration.StartIncludeUncommittedChanges { | |||
if uncommittedChanges && !configuration.StartIncludeUncommittedChanges && !configuration.StartDiscardUncommittedChanges { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as you noticed by implementing this feature, its a flag for the user - but for us internally, its not a flag, but one of many options how to deal with uncommitted changes. So I think we should also code it this way.
the flags should result in some sort of enum
DealWithUncommittedChanges {
Include,
Discard,
Ignore
}
where each of it leads to another path in our code.
This also eliminates the invariant where both booleans could be true (which doesnt make any sense).
The names I used in the enum are just quickly made up. You might find better names, or a better solution to implement the polymorphism, but please don't use two booleans for the same decision point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregorriegler I think you are right. It should be a enum. I adjusted it. Please have a look again :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like it! feel free to merge
Co-authored-by: Dario Zanotto <[email protected]> Co-authored-by: Martin Sereinig <[email protected]> Co-authored-by: Harald Reingruber <[email protected]> Co-authored-by: Michael Weichselbaumer <[email protected]> Co-authored-by: Ricardo Colombo <[email protected]> Co-authored-by: Bernadette Hammerle <[email protected]>
Unified configuration handling for uncommitted changes into `HandleUncommittedChanges` and added corresponding constants. Updated tests and logic to reflect this new approach, ensuring better maintainability and clarity.
c74f57e
to
d2433c8
Compare
This PR closes #446