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

tracing-attributes: support const values for target and name #2941

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Apr 20, 2024

Fixes: #2460

I only fixed lints making #2917 fail. Seems like they affect master branch as well?

Copy link
Contributor

@hds hds left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

There are a couple of comments I've added regarding test layout and the code comment. Also, it would be best to separate the changes to make clippy pass into a separate PR (e.g. #2814).

tracing-attributes/src/attr.rs Outdated Show resolved Hide resolved
tracing-attributes/tests/instrument.rs Outdated Show resolved Hide resolved
tracing-core/src/dispatch.rs Outdated Show resolved Hide resolved
@dpc dpc requested a review from hds April 22, 2024 18:57
@dpc dpc force-pushed the 24-04-19-pr-2917-fix branch 2 times, most recently from d267cca to 2ddad6f Compare April 23, 2024 22:11
@dpc dpc requested a review from mbrobbel May 2, 2024 22:13
@dpc
Copy link
Contributor Author

dpc commented May 10, 2024

I miss this feature so much, every day. :D

What's the best way to give it another review, approval, merge, release? :D

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for addressing the feedback from @hds!

@hawkw hawkw enabled auto-merge (squash) May 10, 2024 20:07
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.

Const value as target = value in instrument.
4 participants