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.

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

Pass instance of Diagnostics.Process for addOnStarted #2451

Merged

Conversation

maciej-izak
Copy link

Description

Pass instance of Diagnostics.Process for created process for function used in addOnStarted
This feature was approved in gitter

@matthid
Copy link
Member

matthid commented Jan 13, 2020

Ok, but question is if we should add a new method instead as the current change is technically breaking all the following usages:

cp |> addOnStarted (fun () -> ())

And it is binary breaking (which probably kind of matters for the Process module as it is used everywhere)

@maciej-izak
Copy link
Author

maciej-izak commented Jan 13, 2020

@matthid You are right. My proposition for new method is addOnProcessStarted. What you think?

@matthid
Copy link
Member

matthid commented Jan 13, 2020

Yes or 'addOnStartedEx' with a record which we can extend in the future?

In any case thanks and congratulations to your first PR :)

…which can be extended in the future

* rollback changes for addOnStarted
@maciej-izak
Copy link
Author

@matthid addOnStartedEx is ready. I have hope it is fine, but if in some point is not good enough let me know, this is my first PR in F# world ever :)

matthid
matthid previously approved these changes Jan 14, 2020
Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

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

Looks good, lets see if my changes on the web actually compile.

@matthid
Copy link
Member

matthid commented Jan 16, 2020

Thanks a lot!

@matthid matthid merged commit 48f0c8b into fsprojects:release/next Jan 16, 2020
@matthid matthid mentioned this pull request Feb 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants