Group pkg documentation #81
Conversation
Sypheos
requested review from
karitham and
lordteka
and removed request for
a team
|
I really like the idea, but I would prefer if we'd put the examples in actual example functions (we have a language feature for it!) https://go.dev/blog/examples, instead of having these huge doc comments |
|
The problem with using function for examples is that they need to compile 🙃 . Also they only show up if you use the web interface, they won't show with just |
karitham
left a comment
karitham
left a comment
There was a problem hiding this comment.
That's fair, gopls does not expose them so we can only really see them by going to pkg.go.dev (in the case of this public package) or using go doc -http github.com/upfluence/pkg/group. Apparently goland does expose them but we can't really rely on that. LGTM I guess
tgallice
left a comment
tgallice
left a comment
There was a problem hiding this comment.
I think we could also add testable examples like alexis did in the errors pkg. https://github.com/upfluence/errors/pull/16/files#diff-d25f78641f53ef1d158d846c90d4162de71971d2311b554d7ae3a40b86456355
This as the benefice to ensure that examples are functional, AI trend to "invent" methods or functions ^^
Otherwise LGTM
| // throttled := group.ThrottledGroup(g, 10) // max 10 concurrent | ||
| // | ||
| // for _, item := range items { | ||
| // item := item |
There was a problem hiding this comment.
We really need to configure a agent for generated doc in order to add some rules to respect like use go version/style etc...
There was a problem hiding this comment.
Yeah, I already removed quite a lot 😢
group/limiter/group.go
Outdated
| // // Limit API calls to 50 per second | ||
| // rateLimiter := rate.NewLimiter(rate.Limit(50), 10) | ||
| // | ||
| // g := limiter.WrapGroup( |
There was a problem hiding this comment.
It's a bit duplicate with the global exemple L11
There was a problem hiding this comment.
🤔 I can remove the wrapping doc ?
upfluence-bot
added
reviewed/needs-work
and removed
reviewed/ready-to-merge
reviewed/needs-work
labels
I didn't add them as they on purpose see. As for the hallucination, it's been a while since I faced any of them 🤔 |
|
@Sypheos the advantage i see with testable Example, is that it could be validated as part of the CI. As the code base evolves public symbol can have breakage in behavour or API, in that case having automated assertion of the validity of the example is pretty neat. Bonus point: It increases the coverage! |
group/group.go
Outdated
| // It receives a context.Context for cancellation and deadline support, | ||
| // and returns an error if the operation fails. | ||
| // | ||
| // Example: |
There was a problem hiding this comment.
if you provide example as testable example, should you remove them from here?
upfluence-bot
added
reviewed/ready-to-merge
reviewed/needs-review
and removed
reviewed/needs-review
reviewed/ready-to-merge
labels
group/group.go
Outdated
| // until all scheduled runners complete, returning any errors according to | ||
| // the implementation's strategy. | ||
| type Group interface { | ||
| // Do schedule a Runner to execute concurrently. |
There was a problem hiding this comment.
I think this was mistakenly reverted; 3rd person present of "to schedule" is "schedules"
| // Do schedule a Runner to execute concurrently. | |
| // Do schedules a Runner to execute concurrently. |
There was a problem hiding this comment.
Grammar checker replaced by schedule because it though Do is a verb 🥲
upfluence-bot
added
reviewed/needs-work
and removed
reviewed/needs-review
reviewed/needs-work
labels
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on December 28. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
What does this PR do?
Add code examples and doc for the group package
What are the observable changes?
Good PR checklist
Additional Notes
based on the idea of @tgallice
https://github.com/upfluence/sql/compare/tg/doc?expand=1&short_path=b335630#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5
Note
Adds comprehensive documentation and example tests for group concurrency patterns (error, wait, throttled, limiter, typed, map).
group/group.goand detailed comments forErrorGroup,ExitGroup,WaitGroup,ThrottledGroup,SharedThrottledGroup,TypedGroup,TypedRunner,MapRunner, andExecuteMap.group/limiter/group.go.group/err_group_example_test.go: examples forExitGroupandErrorGroup.group/wait_group_example_test.go: example forWaitGroupcollecting errors.group/throttled_group_example_test.go: example forThrottledGroup.group/limiter/example_test.go: example forlimiter.WrapGroupwith rate limiter.group/map_example_test.go: example forExecuteMap.group/typed_group_example_test.go: examples forTypedGroupandTypedRunner.group/limiter/group.go(method formatting).Written by Cursor Bugbot for commit 1c6cd16. This will update automatically on new commits. Configure here.