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

Rename Start() to Run() since it's a blocking call #615

Closed
wants to merge 2 commits into from
Closed

Rename Start() to Run() since it's a blocking call #615

wants to merge 2 commits into from

Conversation

nilebox
Copy link
Member

@nilebox nilebox commented Mar 10, 2020

Rename Start() to Run() to follow the naming pattern used in Go, e.g. see blocking Cmd.Run vs non-blocking Cmd.Start.

Same as #607 but keeping original method for backward compatibility. We can keep it for a few releases before deleting.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

Start really gives the impression that it won't wait for the app to exit (see components Start methods). Since the break (when Start is removed) is trivial I'm for it.

// given by the user.
// given by the user, and waits for it to complete.
//
// Deprecated: use Run instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

This is a breaking change of an exported public function. The improvement is not worth it.
Ignore this, I misunderstood the change.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

However, I still think it is not worth it. We end up with 2 functions. It is even more confusing than just using the name Start instead of Run.

@owais
Copy link
Contributor

owais commented Mar 11, 2020

I think it's worth it given the minimal impact it'll have now vs confusion it might create a year later. I'd even go as far as returning an error from Start() that says the developer should use Run() instead. It's a minimal, low-impact change and I don't think it'll result in anyone spending any more time on upgrading their collector dependency that they already would.

@tigrannajaryan
Copy link
Member

I am going to close this as "won't accept" unless someone has arguments which I am missing.

This change adds an duplicate function that does the same thing as another existing function. What's the end game? Are we keeping both? If we intend to retire one it means more work needs to be done, we need to announce a breaking change and fix contrib that relies on Start. I think these all is not worth the time everyone has to spend on it. Start is an OK name, not ideal, but will do.

@nilebox your contributions are highly welcome and I believe you are already contributing much higher value PRs by doing the OTLP implementation. I hope you don't mind if I block low ROI items like this, happy to discuss further to explain my position if it is not clear.

@tigrannajaryan
Copy link
Member

Closing since I don't see objections. We can reopen if needed.

@nilebox nilebox deleted the nilebox/start-run branch June 5, 2020 03:49
@nilebox
Copy link
Member Author

nilebox commented Jun 22, 2020

If we intend to retire one it means more work needs to be done, we need to announce a breaking change and fix contrib that relies on Start. I think these all is not worth the time everyone has to spend on it.

As someone who has spent significant time to rebase our internal code on v0.3.0 and then v0.4.0 and address all breaking changes, I still think that this particular change would have been very small and harmless.

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.

4 participants