Skip to content

Conversation

@delfiterradas
Copy link

Copy link
Owner

@pinin4fjords pinin4fjords left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! The changes improve error messaging and reduce verbosity effectively. I have a few suggestions:

R/accessory.R - Minor spacing inconsistency

In the error message formatting, there's an inconsistency in the paste() separators:

  • missing_vars uses ", " (comma + space)
  • reference_list uses "," (comma only)

Consider making them consistent for better readability:

"Available ", reference_list_name, ": ", paste(unique(reference_list), collapse = ", ")

exec/validate_fom_components.R - Overly aggressive suppression?

The library loading now uses:

suppressMessages(suppressWarnings(library(shinyngs, quietly = TRUE, warn.conflicts = FALSE)))

This seems redundant and potentially too aggressive:

  • quietly = TRUE already suppresses messages, making suppressMessages() redundant
  • warn.conflicts = FALSE already handles conflicts
  • suppressWarnings() will hide ALL warnings, including potentially legitimate ones

Consider using just:

library(shinyngs, quietly = TRUE, warn.conflicts = FALSE)

This should achieve the goal of reducing noise while still allowing genuine warnings to surface if needed.

Overall

The core improvement to checkListIsSubset() is excellent - showing only missing variables instead of all tested values makes the error messages much more actionable. Nice work addressing the verbosity issue!

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.

Make output more concise in VALIDATOR process

2 participants