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

[IMPROVED] Documentation of jetstream options and Fetch clarification #1770

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

piotrpio
Copy link
Collaborator

Resolves #1756

Signed-off-by: Piotr Piotrowski [email protected]

@piotrpio piotrpio requested a review from Jarema January 11, 2025 19:33
@coveralls
Copy link

coveralls commented Jan 11, 2025

Coverage Status

coverage: 84.898% (-0.05%) from 84.949%
when pulling aa516e0 on docs-inprovement
into 1e1519f on main.

Copy link
Member

@Jarema Jarema 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, but I wonder if phrasing could be bit better.

Comment on lines 107 to 109
//
// PullMaxMessages implements PullConsumeOpt and PullMessagesOpt and thus can
// be used to configure both Consumer.Consume and Consumer.Messages.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//
// PullMaxMessages implements PullConsumeOpt and PullMessagesOpt and thus can
// be used to configure both Consumer.Consume and Consumer.Messages.
// PullMaxMessages implements both PullConsumeOpt and PullMessagesOpt, allowing it to configure Consumer.Consume and Consumer.Messages.

How about such phrasing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Piotr Piotrowski <[email protected]>
@piotrpio piotrpio requested a review from Jarema January 14, 2025 16:49
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!

@piotrpio piotrpio merged commit f50c665 into main Jan 15, 2025
6 checks passed
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.

Documentation on PullConsumeOpt and PullMessagesOpt is not clear
3 participants