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

[Improvement] Throw in yield function could result in unexpected behavior #2755

Closed
Evangelink opened this issue Jul 15, 2021 · 2 comments · Fixed by #2756
Closed

[Improvement] Throw in yield function could result in unexpected behavior #2755

Evangelink opened this issue Jul 15, 2021 · 2 comments · Fixed by #2756
Milestone

Comments

@Evangelink
Copy link
Contributor

I would like to suggest a small change in the GetBranchesContainingCommit yielding function. Its current form makes it fully "asynchronous" causing the exception to be thrown only when the collection will be iterated. That's currently not a problem in the codebase because all call sites are doing direct iteration but this could cause some issue in the future if new call sites are added without respecting this condition.

Detailed Description

Please use the following Sharplab repro to see the problem live.

Context

As said in the description, there is no direct problem but this is an issue to become if people are unaware of this behavior and so do not respect the condition use.

Possible Implementation

The sharplab link contains a suggestion of implementation (with a local function), the name has to be defined. I usually go for Iterator, Impl or function name + suffix (e.g. XXXIterator or XXXImpl). It's also probably worth adding a comment to explain the reasoning of the split.

@asbjornu
Copy link
Member

Thanks for pointing it out. A PR with your possible implementation would be welcome. 🙏🏼

@github-actions
Copy link

🎉 This issue has been resolved in version 5.6.11 🎉
The release is available on:

Your GitReleaseManager bot 📦🚀

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

Successfully merging a pull request may close this issue.

2 participants