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

Add 'spanId' to Trace interface #805

Merged
merged 3 commits into from
Jun 7, 2023

Conversation

gstro
Copy link
Contributor

@gstro gstro commented May 14, 2023

Looks like spanId was omitted from the Trace[F] interface, probably by accident? According to this section of the docs, it should be accessible from the ambient spans using the Trace effect.

Add `Applicative` constraint to new `spanId` field in interface to provide default for backwards compatibility.

Co-authored-by: Arman Bilge <[email protected]>
@cb372
Copy link
Contributor

cb372 commented May 19, 2023

Nice one Greg! I need this and I was about to make an identical PR.

@gstro
Copy link
Contributor Author

gstro commented May 20, 2023

@armanbilge Any chance to get this approved? Or someone else I should ping?

@armanbilge
Copy link
Member

Any chance to get this approved?

Does this actually compile for you?

@gstro
Copy link
Contributor Author

gstro commented May 20, 2023

Does this actually compile for you?

Oof, no, sorry! I just assumed the recommended change would be enough. I'll work on getting this ready!

@gstro
Copy link
Contributor Author

gstro commented May 20, 2023

Ok, good to go now!

@gstro
Copy link
Contributor Author

gstro commented May 25, 2023

@armanbilge Sorry to ping again but this is ready for review!

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! (sorry, I'm not an official maintainer here, but I am sure they will take a look soon :)

@armanbilge armanbilge merged commit 7d90540 into typelevel:main Jun 7, 2023
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.

3 participants