Skip to content

Adding print! to the list of specially-formatted format!-like macros #2240

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

Merged
merged 4 commits into from
Dec 7, 2017

Conversation

davidalber
Copy link
Contributor

This PR adds new tests for #549 and corrects a typo from #2219.

  • Added print! to the list of specially-formatted macros.
  • Added tests for each instance of specially-formatted macros.

This commit corrects what appears to be an accidental inclusion of
`panic!` twice in the list resulting from the union of ffbe52e and
aeb3398.
@@ -1818,7 +1818,7 @@ const FORMAT_LIKE_WHITELIST: &[&str] = &[
"eprintln!",
"format!",
"format_args!",
"panic!",
"print!",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expand this snippet below to see that panic! appears in the list further below. Looking deeper now, though, panic! belongs in the part of the list since it is part of the Standard Library. Shall I add it back and keep print! here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please! And thank you very much for adding tests!

@davidalber
Copy link
Contributor Author

I realized that I didn't add similar tests for each of write!, writeln!, and assert!. I'll do that, too.

@topecongiro: log! has syntax that is similar (at least visually) to write! and friends. Shall I add it in for special formatting with write!, writeln!, and assert!?

@topecongiro
Copy link
Contributor

@davidalber Adding more tests is awesome, thank you! The link is to the old version of log! macro. The recent version of log! allows more complex syntax, so I think we shouldn't add it.


// assert!
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 dropped the comments that indicate which macro is being tested because after adding so much duplication I feel the macro names jump out really clearly. I can add comments back if anyone feels strongly about it.

@topecongiro topecongiro merged commit 23fa0bc into rust-lang:master Dec 7, 2017
@topecongiro
Copy link
Contributor

Thank you!

@davidalber davidalber deleted the revisit-2219 branch December 7, 2017 09:10
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.

2 participants