Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

allow custom format on item for Debug #279

Merged
merged 17 commits into from
Aug 5, 2023
Merged

Conversation

ModProg
Copy link
Contributor

@ModProg ModProg commented Jul 25, 2023

Resolves #277

Synopsis

Solution

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

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.

@ModProg thanks! What is missing is a full set of tests to cover this feature.

@tyranron tyranron added this to the 1.0.0 milestone Jul 26, 2023
@tyranron
Copy link
Collaborator

@ModProg @JelteF also, it's a bit unclear how it should work with the item-level formatting attributes?

#[derive(Debug)]
#[debug("{name}={value:?}")]
struct Arg {
  #[debug("{name}!")]
  name: String,
  value: String
}

What should happen here?

  1. Compilation error saying that we cannot mix top-level and field-level formatting attributes.
  2. Print name!=value, considering the field-level formatting in top-level attribute.
  3. Print name=value, silently ignoring the field-level formatting.

I'd vote for the option 1.

@JelteF
Copy link
Owner

JelteF commented Jul 26, 2023

it's a bit unclear how it should work with the item-level formatting attributes?

I think option 1 seems good.

@ModProg
Copy link
Contributor Author

ModProg commented Jul 26, 2023

while 1. is definitly easier, 2. looks reasonable as well. but we can do that later, as it's a non breaking change.

@ModProg ModProg requested a review from tyranron July 30, 2023 16:45
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.

@ModProg at the moment, trait bounds for type parameters are being inferred incorrectly when top-level attribute comes into play. I've added tests covering those situations.

@JelteF
Copy link
Owner

JelteF commented Aug 3, 2023

I've added tests covering those situations.

Somehow github is not showing them in this PR (but I see them on the branch on @ModProg their fork). I'm assuming the tests are not passing?

@ModProg ModProg requested a review from tyranron August 3, 2023 09:04
@ModProg
Copy link
Contributor Author

ModProg commented Aug 3, 2023

@JelteF @tyranron I added it to the bounds_generation. If a format argument is present for the container, its bounds replace the field bounds.

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.

@ModProg thank you! Very nice job and quite a thoughtful implementation! 👍

I've ripped some repeated parts a little bit more, though, and made some bikeshedings regarding errors terminology.

@JelteF would you like to take a look, or we can merge it?

@JelteF JelteF merged commit 6a4772a into JelteF:master Aug 5, 2023
@tyranron
Copy link
Collaborator

tyranron commented Aug 7, 2023

@JelteF could you make a 0.1.0-beta.3 release with fixes of recently merged PRs? I'd like to try it out on our codebase.

@JelteF
Copy link
Owner

JelteF commented Aug 7, 2023

could you make a 0.1.0-beta.3 release with fixes of recently merged PRs?

Done

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

Successfully merging this pull request may close these issues.

Support custom format on item for Debug derive macro
3 participants