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: fix name clash inside of tracing macros #806

Merged
merged 1 commit into from
Jul 12, 2020

Conversation

mainrs
Copy link
Contributor

@mainrs mainrs commented Jul 11, 2020

This commit fixes name clashes that occurred inside of tracing macros when values with the names display, Value and debug have been used.

Motivation

This allows library users to trace variables that have the same name as imported types inside the tracing::field module.

Solution

Use fully qualified type names for types used inside the tracing::field module.

Related issues/PRs

Fixes #805.

This commit fixes name clashes that occured inside of tracing macros when values with the names display,Value,debug have been used.
@mainrs mainrs requested review from hawkw and a team as code owners July 11, 2020 16:41
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 change looks good to me.

If you like, I think it might be good to add the repro from #805 as a test to ensure that there aren't regressions in the future.

Also, a couple procedural notes: can we change the PR title to start with the crate that the change applies to (in this case, "tracing:") rather than "fix:", and add "Fixes #805" to the commit message somewhere? CONTRIBUTING.md has documentation on the preferred convention for commit message/PRs. Not a big deal, of course, just thought I'd mention.

@mainrs
Copy link
Contributor Author

mainrs commented Jul 11, 2020

Also, a couple procedural notes: can we change the PR title to start with the crate that the change applies to (in this case, "tracing:") rather than "fix:", and add "Fixes #805" to the commit message somewhere?

I can do that. Should I just force-push the changes to this PR? Thanks for mentioning the contributions.md. I actually didn't see it 😄

@mainrs mainrs changed the title fix: name clash inside of tracing macros tracing: name clash inside of tracing macros Jul 11, 2020
@mainrs mainrs changed the title tracing: name clash inside of tracing macros tracing: fix name clash inside of tracing macros Jul 11, 2020
@hawkw
Copy link
Member

hawkw commented Jul 11, 2020

You don't need to force push, just edit the PR name and description. We'll squash the whole PR down to one commit when merging.

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.

lgtm

@mainrs
Copy link
Contributor Author

mainrs commented Jul 11, 2020

Done.

@hawkw hawkw merged commit e3b3a3a into tokio-rs:master Jul 12, 2020
@mainrs mainrs deleted the fix_macro_name_clash branch July 12, 2020 08:46
hawkw added a commit that referenced this pull request Jul 21, 2020
This reverts commit e3b3a3a.

This change was accidentally semver-incompatible, as user code was able
to use the imported `debug` and `display` functions without adding an
import outside the macro. Although this was not documented, downstream
code relied on these names being available, so this resulted in a
breaking change in a point release.

Fixes #820
hawkw added a commit that referenced this pull request Jul 21, 2020
This reverts commit e3b3a3a.

This change was accidentally semver-incompatible, as user code was able
to use the imported `debug` and `display` functions without adding an
import outside the macro. Although this was not documented, downstream
code relied on these names being available, so this resulted in a
breaking change in a point release.

Fixes #820
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.

logging a variable called display results in compilation errors
2 participants