Skip to content

compact_list() hangs for large list elements#1197

Merged
strengejacke merged 14 commits into
mainfrom
strengejacke/issue1196
May 24, 2026
Merged

compact_list() hangs for large list elements#1197
strengejacke merged 14 commits into
mainfrom
strengejacke/issue1196

Conversation

@strengejacke

Copy link
Copy Markdown
Member

Fixes #1196

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the insight package to version 1.5.1.1 and refactors the compact_list function to improve performance on large, nested lists by replacing sapply with vapply and introducing a recursive helper. The review identifies a critical bug where the internal helper .is_null_string is referenced incorrectly in a rapply call, which would lead to runtime errors. Other feedback includes a suggestion to maintain consistent capitalization in the NEWS.md file and a request to clarify a misleading code comment.

Comment thread R/utils_compact.R Outdated
Comment thread NEWS.md Outdated
Comment thread R/utils_compact.R Outdated
@strengejacke

Copy link
Copy Markdown
Member Author

@gemini-code-assist review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses a hang in compact_list() when handling large (notably nested) list elements, which can cause downstream functions like performance::check_model() to stall on large model objects.

Changes:

  • Refactors compact_list() to use a new .is_null_string() helper and vapply() for more predictable/efficient evaluation.
  • Adds a depth-based guard intended to avoid expensive as.character() calls on nested lists.
  • Adds tests and documentation clarifying that compact_list() does not recursively compact deeper list levels by default, plus a NEWS entry and version bump.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
R/utils_compact.R Refactors compacting logic and introduces helpers to avoid expensive character coercion on nested lists.
tests/testthat/test-compact-list.R Adds regression tests for behavior with deeper nested lists.
man/compact_list.Rd Documents non-recursive behavior for deeper list levels and adds an example.
NEWS.md Notes a performance improvement for compact_list() on very large nested lists.
DESCRIPTION Bumps package version.
Files not reviewed (1)
  • man/compact_list.Rd: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread R/utils_compact.R Outdated
Comment thread R/utils_compact.R Outdated
Comment thread R/utils_compact.R Outdated

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request enhances the performance of compact_list and compact_character by utilizing vapply and implementing a depth-limited check to avoid costly character coercion on large nested lists. Review feedback identifies opportunities to further optimize the .is_null_string helper by explicitly excluding data frames and removing redundant unlist calls. Additionally, the .large_list_depth function should be refactored to prevent warnings on empty lists and support early termination for efficiency.

Comment thread R/utils_compact.R Outdated
Comment thread R/utils_compact.R Outdated
Comment thread R/utils_compact.R Outdated
Agent-Logs-Url: https://github.com/easystats/insight/sessions/fc8ae76d-f002-4255-b015-0f9306587a09

Co-authored-by: strengejacke <26301769+strengejacke@users.noreply.github.com>
@strengejacke strengejacke merged commit 9b00f19 into main May 24, 2026
18 of 20 checks passed
@strengejacke strengejacke deleted the strengejacke/issue1196 branch May 24, 2026 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compact_list() hangs for large list elements

3 participants