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 support for captured identifiers #182

Merged
merged 17 commits into from
Feb 9, 2022

Conversation

ilslv
Copy link
Contributor

@ilslv ilslv commented Jan 20, 2022

Since Rust 1.58 format_args! macro can capture identifiers: format!("Prefix: {some_local_variable}"). This PR adds support for this feature while preserving MSRV by transforming format!("Prefix: {field}") into format!("Prefix: {field}", field = field).

Limitations

This approach doesn't work well with unnamed fields that are aliased with _0, _1, ... because until 1.41 identifiers beginning with underscore weren't supported

@JelteF
Copy link
Owner

JelteF commented Jan 20, 2022

while preserving MSRV by transforming format!("Prefix: {field}") into format!("Prefix: {field}", field = field).

I haven't looked at the code yet, but if it makes the implementation easier I wouldn't mind if it this new syntax is only supported on 1.58+, as long as the old syntax still works on the current MSRV.

@ilslv
Copy link
Contributor Author

ilslv commented Jan 20, 2022

@JelteF

if it makes the implementation easier I wouldn't mind if it this new syntax is only supported on 1.58+, as long as the old syntax still works on the current MSRV.

Both the new syntax and the old one work fine on MSRV with 1 limitation described in PR message.

I haven't looked at the code yet

This PR depends on a new parser implemented in #181 so we should wait until it's merged.

ilslv added 2 commits February 8, 2022 15:20
# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	src/display.rs
#	src/parsing.rs
@JelteF
Copy link
Owner

JelteF commented Feb 8, 2022

I merged #181. Please update this PR and let @tyranron review it.

@JelteF JelteF requested a review from tyranron February 8, 2022 12:34
@ilslv ilslv marked this pull request as ready for review February 8, 2022 13:05
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@ilslv LGTM, thanks!

src/display.rs Outdated
/// [Width argument][1], if present.
///
/// [1]: https://doc.rust-lang.org/stable/std/fmt/index.html#width
width: Option<Argument>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we better align here with the terminology used in the linked docs, not the one proposed by the formal grammar (we may keep it just for the parsing).

So, Argument - > Parameter, Ident -> Named, Integer -> Positional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support thiserror like formatting for Display derive
3 participants