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

TC-1020: Keep strategy directive around long enough for proper merge #12

Merged
merged 14 commits into from
Jun 30, 2017
Merged

TC-1020: Keep strategy directive around long enough for proper merge #12

merged 14 commits into from
Jun 30, 2017

Conversation

roelandvanbatenburg
Copy link
Collaborator

@roelandvanbatenburg roelandvanbatenburg commented Apr 3, 2017 •
edited by bsander
Loading

Ticket

Link to ticket: https://enrise.atlassian.net/browse/TC-1020

What has been done

  • Only delete the _strategy directive after the merge has completed, not before it starts. This requires all strategy implementations to perform the delete action when they are done.
  • Updated eslint rules

How to test

  • Failing unit test now succeeds
  • Try with case discovered in ticket

@bsander bsander removed their assignment Apr 5, 2017
@bsander bsander self-requested a review April 5, 2017 10:45
@bsander bsander changed the title add test case for nested wildcards TC-1020: Keep strategy directive around long enough for proper merge Apr 11, 2017
@bsander bsander removed their request for review April 11, 2017 14:08
@roelandvanbatenburg roelandvanbatenburg requested review from bsander and removed request for bsander April 13, 2017 09:24
@roelandvanbatenburg
Copy link
Collaborator Author

@bsander I can't review this (probably because I created the PR). Looks good to me, however I'm curious why this hasn't been an issue before.

@roelandvanbatenburg
Copy link
Collaborator Author

@bsander turns out my original test did not quite cover the use case, added another test.

@roelandvanbatenburg
Copy link
Collaborator Author

Btw, if you move the _instances: true on level1 to the icin, it behaves properly

@thabemmz
Copy link
Collaborator

Tomorrows I will write some documentation for this.

@roelandvanbatenburg
Copy link
Collaborator Author

@bsander can't add my review as I started this PR...

bsander
bsander approved these changes Jun 30, 2017
View reviewed changes
Copy link
Owner

@bsander bsander left a comment

Choose a reason for hiding this comment

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

LGTM, is this mergable or still waiting for the docs update? @thabemmz

@roelandvanbatenburg
Copy link
Collaborator Author

Discussed it with @thabemmz documentation is not for now 😞

@roelandvanbatenburg roelandvanbatenburg merged commit c687a35 into master Jun 30, 2017
@bsander bsander deleted the feature/nested_wildcards branch June 30, 2017 12:50
@thabemmz
Copy link
Collaborator

Yeah, I cannot write a README of this that soon. That will have to wait until later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsander bsander bsander approved these changes

Assignees

@bsander bsander

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@roelandvanbatenburg @thabemmz @bsander