Skip to content

Refactoring of merge message class #1487

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

Merged
merged 3 commits into from
Apr 10, 2019
Merged

Refactoring of merge message class #1487

merged 3 commits into from
Apr 10, 2019

Conversation

Kieranties
Copy link
Contributor

@Kieranties Kieranties commented Oct 2, 2018

Refactoring of the MergeMessage class to enable simpler (and consistent) approach to adding new regular expressions. This is a prelude to an additional PR where I hope to enable custom patterns through config.

  • All patterns are now stored in a collection. The collection is enumerated to apply the order in which they are checked (currently the same order previously used)
  • Each pattern has a required SourceBranch capture group
  • Pull request patterns have a required PullRequestNumber capture group
  • All patterns (except Bit Bucket pull request) have an optional TargetBranch capture group. TargetBranch is required for Bit Bucket pull requests

This introduces the following breaking changes:

Merge message Previous result Breaking Change
Merge pull request # from feature/one Pull Request Number = 0
Merged Branch = feature/one
No match
Merge pull request # in feature/one from feature/two to master Pull Request Number = 0
Merged Branch = feature/two
Target Branch = master
No match
Merge pull request #1234 from feature/one into dev Pull Request Number = 1234
Merged Branch = feature/one into dev
Target Branch = dev
Pull Request Number = 1234
Merged Branch = feature/one
Target Branch = dev
Merge pull request #1234 from feature/one from feature/two to master Pull Request Number = 1234
Merged Branch = feature/two
Target Branch = null
Pull Request Number = 1234
Merged Branch = feature/two
Target Branch = master
Finish V4.0.0 into master Merged Branch = V4.0.0 into master
Target Branch = master
Merged Branch = V4.0.0
Target Branch = master

@Kieranties Kieranties changed the title Refactoring of merge message Refactoring of merge message class Oct 2, 2018
@asbjornu
Copy link
Member

asbjornu commented Oct 2, 2018

I think all of the old matches make no sense, so I welcome these breaking changes. I'll wait for others to reply before merging, but this LGTM.

@Kieranties
Copy link
Contributor Author

@asbjornu I've pre-emptively raised #1488 which introduces the actual feature set I've been working towards. It favours KeyValuePair over the custom class but does have a dependency on the breaking changes in this request.

If you're happy with the breaking changes, should I close this and move the breaking change info into that request?

@asbjornu
Copy link
Member

asbjornu commented Oct 3, 2018

@Kieranties: No, we'll keep this open for a few days before merging so @GitTools/developers can have a chance to review it. If I hear nothing, I'll merge and you can rebase #1488 on master afterwards.

@asbjornu
Copy link
Member

@Kieranties: Since we've now decided to bump the major version due to other breaking changes, I would highly appreciate if you could rebase this PR and fix the merge conflicts. 🙏

@Kieranties
Copy link
Contributor Author

@asbjornu I'm tied up for a few days, but will get this done. I'll also rebase/update #1488.

@arturcic arturcic added this to the 5.0.0 milestone Feb 18, 2019
@asbjornu
Copy link
Member

@Kieranties: Excellent!

Kieran Marron and others added 2 commits March 9, 2019 15:38
Add additional tests for TFS messages
@Kieranties
Copy link
Contributor Author

@asbjornu Rebase complete. Let me know of any issues. If the PR gets completed I'll do the follow up work on #1488. There are some additional tests added to cater for the TFS merge messages.

@Kieranties
Copy link
Contributor Author

@asbjornu I see this is out of date again. I've updated my branch to chase things up.

@asbjornu asbjornu merged commit c3ca29e into GitTools:master Apr 10, 2019
@asbjornu
Copy link
Member

Thanks, @Kieranties. 🙏

@Kieranties
Copy link
Contributor Author

Awesome! I'll get #1488 fixed up this evening!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants