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

docs: fix function comment #2947

Open
wants to merge 1 commit into
base: v0.1.x
Choose a base branch
from

Conversation

kaffarell
Copy link
Contributor

The comment on the visit_spans fn is wrong. Change it to the correct order.

Fixes: #2940

The comment on the `visit_spans` fn is wrong. Change it to the correct
order.

Fixes: tokio-rs#2940
@kaffarell kaffarell requested review from hawkw and a team as code owners April 22, 2024 08:27
/// and then with that span's parent, and then that span's parent,
/// and so on until a root span is reached.
/// The provided closure will be called first with the root span,
/// and then traverse through all the children until it reaches
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that "all the children" is potentially misleading as one could take that to mean the children of the root span.

I can't really think of how to make the wording clearer, for reference this is how SpanRef::scope documents this so maybe use something similar (but reverse and with callback)?

    /// Returns an iterator over all parents of this span, starting with this span,
    /// ordered from leaf to root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, how about I just remove the 'all'?
So: '...traverse through the children until it ...'.

I could also write:

Calls the callback function for all the parents of the span, starting with the root span, ordered from root to current.

but IMO this is more confusing.

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.

None yet

2 participants