axum-core/macros: Change Display impl to match body_text() result
#3118
Conversation
It seems less useful to only display `$body` without any information about the inner error. Since `body_text()` already combines both, this commit changes the `Display` impl to display the same.
Since they both result in the same thing we might as well take advantage of one in the other.
Turbo87
force-pushed
the
rejection-display
branch
from
ac549f3 to
c50ff78
Compare
sure, done side question: any thoughts on using e.g. https://git-cliff.org/ to generate the changelogs automatically from the merged pull requests? We've started using that over at /~https://github.com/LukeMathWalker/cargo-manifest with the changelog entries being based on the labels of the merged pull requests and its been working quite well. |
axum-core/CHANGELOG.md
Outdated
| @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| # 0.5.0 | |||
|
|
|||
| - **change:** Change rejection `Display` impl to match `body_text()` result ([#3118]) | |||
There was a problem hiding this comment.
I think this doesn't have enough info. We need to
- mention that this affects all the (composite / enum?) extractors defined by
axumandaxum-extraor move this to those changelogs - state what was different about these before / what's the user-visible change
There was a problem hiding this comment.
I've slightly tweaked it. Is that better?
- state what was different about these before / what's the user-visible change
I don't think there is a user-visible change in most cases since the body_text() impl hasn't really changed. The only thing that changed is the Display impl, which probably isn't actually visible to the user in most cases.
There was a problem hiding this comment.
I meant how the change would be observable by users of axum, not (necessarily) end users.
New entry looks good 👍
axum-core/src/macros.rs
Outdated
| @@ -132,7 +132,7 @@ macro_rules! __define_rejection { | |||
|
|
|||
| impl std::fmt::Display for $name { | |||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |||
| write!(f, "{}", $body) | |||
| write!(f, concat!($body, ": {}"), self.0) | |||
There was a problem hiding this comment.
I'd slightly prefer f.write_str to not rely on inlining as much, but that probably affects other places too and can be a separate PR once again.
There was a problem hiding this comment.
I've added another commit that switches it over to use write_str()
There was a problem hiding this comment.
Ah, I thought it was just going to be one write_str, but I guess that would have only been the case for the non-composite case. Anyways, doesn't hurt.
Turbo87
force-pushed
the
rejection-display
branch
from
c50ff78 to
b0be0d2
Compare
Yes, I hate it with a passion 😂 Frequently I write changelog entries that look a lot different than the associated commit message, and not because one or the other is bad. They're just for different audiences: commit messages are for the project's developers mostly, changelog entries are for its users. It's also super easy to ask a contributor to add an entry to a markdown file and have the existing contents there guide them to writing a useful entry, or post a follow-up PR adding a previous PR to the changelog. I haven't seen a nice equivalent for those things in projects using git-cliff. I heard that my ex colleagues from my previous job recently tried a git-cliff based workflow where there were separate git commit message trailers for changelog entries, alleviating my first concern from above. However they went back to a hand-written changelog after not too long. Idk why, but if you're interested I can ping one of them 🙂 |
😂 FWIW I agree with you with regard to changelogs generated from commits. That's why I'm usually generating the changelog from pull requests, and specifically their title and labels, which are still adjustable even after they are merged.
sure, though it requires at least one other review cycle, potentially causing delays in merging PRs. if all maintainers are as responsive as you are then this is probably not an issue, but my experience is different 😅 |
It seems less useful to only display
$bodywithout any information about the inner error. Sincebody_text()already combines both, this commit changes theDisplayimpl to display the same.