Fix isVisible() initial value bug in NestedListStringFormatter#1717
Fix isVisible() initial value bug in NestedListStringFormatter#1717alganet merged 1 commit intoRespect:3.1from
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.1 #1717 +/- ##
============================================
- Coverage 99.45% 99.45% -0.01%
- Complexity 1015 1016 +1
============================================
Files 194 194
Lines 2384 2383 -1
============================================
- Hits 2371 2370 -1
Misses 13 13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'm going to use this PR to test panda benchmark. We don't need a benchmark here, but I never used it in a real PR. |
|
@TheRespectPanda benchmark |
|
Failed to trigger benchmarks: Resource not accessible by personal access token |
|
I made a note about the panda failure on #1635 and will work on it later. Reviewers: just forget about this benchmark thing here 😄 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The array_reduce in isVisible() used `true` as its initial value with an `||` reducer, meaning it always returned `true` when siblings existed regardless of their actual visibility. This caused unnecessary nesting in full messages by showing single-child wrapper nodes that should have been collapsed. Replace array_reduce with a foreach loop starting from `false`, which also enables early return on the first visible sibling.
The array_reduce in isVisible() used
trueas its initial value with an||reducer, meaning it always returnedtruewhen siblings existed regardless of their actual visibility. This caused unnecessary nesting in full messages by showing single-child wrapper nodes that should have been collapsed.Replace array_reduce with a foreach loop starting from
false, which also enables early return on the first visible sibling.The only test affected is
AssertWithPropertiesTest, others pass cleanly. If you look at that test history, it seems that the nesting that was present there is a behavior that drifted with time (it existed before full paths likemysql.hostwere possible), and the implementation somehow got super complicated in order to pass a test that really doesn't make sense anymore: collapsing the orphan makes for a much cleaner message and faster runtime with an early return instead of expensive reduce.This arguibly is a BC break, since it changes behavior. However, given the history laid down above, I would prefer to classify it as a bug to be fixed on the 3.1 series.