Updated Unreleased CHANGELOG entries #5305
Conversation
ytmimi
force-pushed
the
update_changelog
branch
2 times, most recently
from
7ecc7b6 to
eb4f38d
Compare
There was a problem hiding this comment.
Thanks so much for pulling this together! I know what a pain it can be 😄
I've made it through most of the first section with some inline comments but have opted to take a pause and go ahead and share the feedback given the recurrence.
To take a step back, I try to keep the target audience in mind when adding content, that of a rustfmt user or rustfmt consumer. I realize we've had a ton of PRs since the last tag and that you've tried to cover all of them, but I think many of these things are irrelevant with that user persona in mind.
I think that users are best served by being able to quickly and easily read through these change/release notes and grok what's relevant for them, and I think that's a far too difficult task if we include all the code/repo level changes and refactorings we've made that users will never see.
Would you be up for taking another pass at this current list of changes and filtering out everything that's not user-facing (i.e. things that a rustfmt user could/would see)?
CHANGELOG.md
Outdated
| - Ensure `cargo-fmt` tests are excluded from the root workspace [PR #5043](https://github.com/rust-lang/rustfmt/pull/5043) | ||
| - Address various clippy and rustc warnings | ||
| - [PR #5040](https://github.com/rust-lang/rustfmt/pull/5040) | ||
| - [PR #5135](https://github.com/rust-lang/rustfmt/pull/5135) | ||
| - [PR #5164](https://github.com/rust-lang/rustfmt/pull/5164) |
There was a problem hiding this comment.
I'd recommend we nix this as I prefer to avoid adding entries to the changelog which are only relevant to rustfmt developers.
The other alternative I've seen which I might be open to is introducing an "Internal" section which squirrels away this type of code-level detail that's not user facing nor user impacting
CHANGELOG.md
Outdated
| - [PR #5040](https://github.com/rust-lang/rustfmt/pull/5040) | ||
| - [PR #5135](https://github.com/rust-lang/rustfmt/pull/5135) | ||
| - [PR #5164](https://github.com/rust-lang/rustfmt/pull/5164) | ||
| - Minor parser cleanup [PR #5056](https://github.com/rust-lang/rustfmt/pull/5056) |
There was a problem hiding this comment.
Same as the above, would recommend we remove entirely or move to a separate internal-style section
CHANGELOG.md
Outdated
| - Various README changes | ||
| - [PR #5048](https://github.com/rust-lang/rustfmt/pull/5048) | ||
| - [PR #5110](https://github.com/rust-lang/rustfmt/pull/5110) | ||
| - [PR #5221](https://github.com/rust-lang/rustfmt/pull/5221) |
CHANGELOG.md
Outdated
| - [PR #5110](https://github.com/rust-lang/rustfmt/pull/5110) | ||
| - [PR #5221](https://github.com/rust-lang/rustfmt/pull/5221) | ||
| - Use `RUSTFMT_LOG` env var instead of `RUST_LOG` to configure rustfmt log output [PR #5051](https://github.com/rust-lang/rustfmt/pull/5051) | ||
| - Update IntelliJ Integration documentation [PR #5049](https://github.com/rust-lang/rustfmt/pull/5049) |
CHANGELOG.md
Outdated
| - Dedupe and simplify type alias formatting [PR #5068](https://github.com/rust-lang/rustfmt/pull/5068) | ||
| - Dedupe associated item visitation [PR #5069](https://github.com/rust-lang/rustfmt/pull/5069) | ||
| - Dedupe and clean up Impl handling [PR #5087](https://github.com/rust-lang/rustfmt/pull/5087) |
CHANGELOG.md
Outdated
| - Dedupe and simplify type alias formatting [PR #5068](https://github.com/rust-lang/rustfmt/pull/5068) | ||
| - Dedupe associated item visitation [PR #5069](https://github.com/rust-lang/rustfmt/pull/5069) | ||
| - Dedupe and clean up Impl handling [PR #5087](https://github.com/rust-lang/rustfmt/pull/5087) | ||
| - Update naming conventions for AST structures [PR #5070](https://github.com/rust-lang/rustfmt/pull/5070) |
CHANGELOG.md
Outdated
| - Dedupe associated item visitation [PR #5069](https://github.com/rust-lang/rustfmt/pull/5069) | ||
| - Dedupe and clean up Impl handling [PR #5087](https://github.com/rust-lang/rustfmt/pull/5087) | ||
| - Update naming conventions for AST structures [PR #5070](https://github.com/rust-lang/rustfmt/pull/5070) | ||
| - Update how the fn params span is calculated to avoid issues with `T: Fn()` bounds [PR #5121](https://github.com/rust-lang/rustfmt/pull/5121) |
There was a problem hiding this comment.
Let's remove this too, there's no need to incorporate subtree syncs nor callout changes made to rustc libs that accordingly required minor changes to rustfmt source
CHANGELOG.md
Outdated
| - Dedupe and clean up Impl handling [PR #5087](https://github.com/rust-lang/rustfmt/pull/5087) | ||
| - Update naming conventions for AST structures [PR #5070](https://github.com/rust-lang/rustfmt/pull/5070) | ||
| - Update how the fn params span is calculated to avoid issues with `T: Fn()` bounds [PR #5121](https://github.com/rust-lang/rustfmt/pull/5121) | ||
| - Block indent line breaks for type alias impl traits (TAITs) when `version = "Two"` [#5027](https://github.com/rust-lang/rustfmt/issues/5027) |
There was a problem hiding this comment.
This would count as a "Fix" I think right?
CHANGELOG.md
Outdated
| - Update naming conventions for AST structures [PR #5070](https://github.com/rust-lang/rustfmt/pull/5070) | ||
| - Update how the fn params span is calculated to avoid issues with `T: Fn()` bounds [PR #5121](https://github.com/rust-lang/rustfmt/pull/5121) | ||
| - Block indent line breaks for type alias impl traits (TAITs) when `version = "Two"` [#5027](https://github.com/rust-lang/rustfmt/issues/5027) | ||
| - Toggle `CFG_RELEASE_CHANNEL` between `nightly` and `stable` during CI runs to simulate stable and nightly release behavior while testing [PR #5109](https://github.com/rust-lang/rustfmt/pull/5109) |
There was a problem hiding this comment.
Another Internal or remove item
CHANGELOG.md
Outdated
| - Update how the fn params span is calculated to avoid issues with `T: Fn()` bounds [PR #5121](https://github.com/rust-lang/rustfmt/pull/5121) | ||
| - Block indent line breaks for type alias impl traits (TAITs) when `version = "Two"` [#5027](https://github.com/rust-lang/rustfmt/issues/5027) | ||
| - Toggle `CFG_RELEASE_CHANNEL` between `nightly` and `stable` during CI runs to simulate stable and nightly release behavior while testing [PR #5109](https://github.com/rust-lang/rustfmt/pull/5109) | ||
| - Maintain additional AST metadata when formatting a RHS [PR #5113](https://github.com/rust-lang/rustfmt/pull/5113) |
There was a problem hiding this comment.
Another Internal or remove
|
Thanks for the review! I know it was a lot to go through even if, as you said, you didn't go through it all. I guess being my first time updating the rustfmt CHANGELOG I wanted to make sure I was being overly inclusive not to miss something that might have been important. That said, you're right and I definitely included plenty that's only relevant for developers and I'm happy to take another pass and trim down what I've got here 😁. Will go through what I've got here later this week. |
ytmimi
force-pushed
the
update_changelog
branch
from
eb4f38d to
89200f8
Compare
|
@calebcartwright ready for a follow up review! |
CHANGELOG.md
Outdated
|
|
||
| - Also apply `empty_item_single_line=true` to trait definitions [#5047](https://github.com/rust-lang/rustfmt/issues/5047) | ||
| - Replace `structopt` dependency with `clap` [PR #5239](https://github.com/rust-lang/rustfmt/pull/5239) |
There was a problem hiding this comment.
In my opinion this is still an internally facing change that's not of particular interest for end users. I suspect part of the friction here is trying to balance the benefits of calling out the work that's been contributed vs. a minimal/succinct/clear/etc. summary of changes relevant for end users.
I don't want to add any additional overhead on ourselves given how we've struggled to maintain this already, but wonder if having a separate changelog (broader content included) vs. release notes (strictly end user focused) would help?
wdyt @ytmimi?
There was a problem hiding this comment.
For now I've decided that we don't need to include the structop dependency change since it's internal and I didn't want to block the PR on this
CHANGELOG.md
Outdated
| - Handle external mods imported via external->inline load hierarchy [#5063](https://github.com/rust-lang/rustfmt/issues/5063) | ||
| - Resolve sub modules of integration tests [#5119](https://github.com/rust-lang/rustfmt/issues/5119) | ||
| - Module resolution will fallback to the current search directory if a relative directory search results in a `FileNotFound` error [#5198](https://github.com/rust-lang/rustfmt/issues/5198) | ||
| - Give clearer error messsaeg when a module is found in two places (e.g `x.rs` and `x/mod.rs`) [#5167](https://github.com/rust-lang/rustfmt/issues/5167) |
There was a problem hiding this comment.
Typo
| - Give clearer error messsaeg when a module is found in two places (e.g `x.rs` and `x/mod.rs`) [#5167](https://github.com/rust-lang/rustfmt/issues/5167) | |
| - Give clearer error message when a module is found in two places (e.g `x.rs` and `x/mod.rs`) [#5167](https://github.com/rust-lang/rustfmt/issues/5167) |
CHANGELOG.md
Outdated
| - Prevent rustfmt from always adding an empty comment line when formatting itemized blocks [#5088](https://github.com/rust-lang/rustfmt/issues/5088) | ||
| - Don't format files annotated with an inner `#![rustfmt::skip]` attribute [PR #5094](https://github.com/rust-lang/rustfmt/pull/5094) | ||
| - Remove duplicate comma when struct pattern ends with `..` and `trailing_comma=Always` [#5066](https://github.com/rust-lang/rustfmt/issues/5066) | ||
| - Fix static async closure qualifier order [#5149](https://github.com/rust-lang/rustfmt/issues/5149) |
There was a problem hiding this comment.
This seems like a good place to make note of a general observation I'm having. Note that I'm not asking for any changes here, just sharing a thought for us to bounce around going forward around how we structure things.
If I'm a user perusing this list, many of these don't really tell me much, and I need to click in (at least once, if not more) to get to a location that has more details which I then need to digest, and sometimes the linked issue or PR doesn't have much information either.
I think it would be nice in the future if we can articulate these in such a way that a Rust dev could read and say "ah okay I understand what the problem was and/or what the change fixed" without having to do that subsequent deep dive. However, that could be better suited for the release notes type of model if we decide to split as I noted above.
There was a problem hiding this comment.
I'm totally open to going back and making edits! I'm sure I can better articulate some of these as well. I agree that a lot of them don't give you a full understanding of the issue and the fix. I think I was trying to limit myself to a one sentence description, but some of these might need a little more than that.
On the topic of internal changes, I remember you mentioned the idea of introducing an Internal changes section. I think it would be nice to include and I wonder if we could follow a similar model as rust-analyzer. See their changelog-127 for reference.
Sticking with the rust-analyzer changelog just a little longer, I also like what they include images. rustfmt's output is also visual so we might be able to do something like that too. Overkill for some issues, but maybe for some we could include a side by side comparison of how rustfmt formatted a code snippet before and after the fix (maybe using two side by side GIFs??).
To help people more easily grok the changes maybe we could also try to logically group issues into categories based on the fix or the issue label we've assigned it? for example, grouping all the a-comment related fixes together in their own subsection under the ### Fixed subheading.
There was a problem hiding this comment.
That certainly sounds intriguing and helpful! The released-but-not-documented changes/fixes/enhancements/etc. are really starting to stack up on us though, so let's try to get things caught up within our current model and continue the dialog on how to make things better going forward
|
How are we looking on this? We desperately need to run another two way sync of the subtree, but I'd really like to do the version bump with associated changelog as part of that |
|
Got it! I'll set aside some time today to make some more updates. Sorry that I've let this sit here for a while |
from revision v1.4.38..2d9bc46
ytmimi
force-pushed
the
update_changelog
branch
3 times, most recently
from
13d0862 to
b81d6c9
Compare
|
@calebcartwright I've gone ahead and made some updates. Definitely let me know if any of the descriptions are unclear or need additional context. I also think I've found and fixed all typos, but please point them out if you see them. In general if anything looks off just let me know and i'll be happy to quickly go back and make the necessary changes |
|
Thanks! I may make some additional tweaks but no need to block/better to get it merged to minimize any more merge conflicts |
|
Let me know if you'd like to see a follow up PR to make any tweaks that you're thinking about |
from revision v1.4.38..2d9bc46