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

Clean up push impact invocation #703

Merged
merged 2 commits into from
Apr 5, 2019
Merged

Clean up push impact invocation #703

merged 2 commits into from
Apr 5, 2019

Conversation

johnsonr
Copy link
Contributor

@johnsonr johnsonr commented Apr 5, 2019

Corrects an error in PushImpactListener registration. Previously it was impossible to return void from such a listener, which was undesirable. Also the types were incorrect in returning a response code.

Rod Johnson and others added 2 commits April 5, 2019 19:46
Clean up PushImpactListenerRegistration
e105783
@atomist-bot
Autofix: TypeScript header
84fbfae 
[atomist:generated] [atomist:autofix=typescript_header]
@johnsonr johnsonr requested a review from cdupuis April 5, 2019 08:53
@cdupuis cdupuis added auto-merge-method:squash Auto-merge with squash and merge auto-merge:on-approve Auto-merge on review approvals breaking Mark this issue or pull request as breaking changelog:changed Add this issue or pull request to changed changelog section labels Apr 5, 2019
@johnsonr
Copy link
Contributor Author

johnsonr commented Apr 5, 2019

Note that although this is technically breaking, I don't think it will break any valid user code. Putting the return type in the object with a response property as the typings required did not work. The tests test for a direct response code return as the present implementation uses.

cdupuis
cdupuis approved these changes Apr 5, 2019
View reviewed changes
Copy link
Member

@cdupuis cdupuis left a comment

Choose a reason for hiding this comment

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

LGTM

Technically this is breaking backwards compatibility but that API was obviously broken. I think this ok to release as a minor version rev.

@atomist-bot atomist-bot merged commit cc00d30 into master Apr 5, 2019
@atomist-bot
Copy link
Contributor

Pull request auto merged by Atomist.

  • 1 approved review by @cdupuis
  • 2 successful checks

[atomist:generated] [auto-merge:on-approve]

@atomist-bot atomist-bot deleted the cleanup-push-impact-invocation branch April 5, 2019 12:24
atomist-bot added a commit that referenced this pull request Apr 5, 2019
@atomist-bot
Changelog: #703 to changed
f8d1bdb 
[atomist:generated]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdupuis cdupuis cdupuis approved these changes

Assignees
No one assigned
Labels
auto-merge:on-approve Auto-merge on review approvals auto-merge-method:squash Auto-merge with squash and merge breaking Mark this issue or pull request as breaking changelog:changed Add this issue or pull request to changed changelog section
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@johnsonr @atomist-bot @cdupuis