Skip to content

Issue-48: Update extract_ids_from_metadata() to accept logical values - #49

Merged
johanna0321 merged 2 commits into
mainfrom
issue-48
Feb 27, 2026
Merged

Issue-48: Update extract_ids_from_metadata() to accept logical values#49
johanna0321 merged 2 commits into
mainfrom
issue-48

Conversation

@johanna0321

@johanna0321 johanna0321 commented Feb 27, 2026

Copy link
Copy Markdown
Contributor

Description of your changes

Update extract_ids_from_metadata() to accept logical values and style files

Issue ticket number and link

#48 for https://github.com/calico/mass_spec/issues/1889

Type of change

  • Bug fix
  • New feature
    • Backwards Incompatible?
  • Refactoring / code clean-up
  • Documentation add / update
  • Automated Test
  • Other (please specify)

(If applicable) How has this been tested?

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 pull request updates the extract_ids_from_metadata() function to accept logical values for the filter_ids parameter, in addition to character vectors, and applies consistent code styling across multiple R files.

Changes:

  • Added support for logical filter_ids in extract_ids_from_metadata() with type validation
  • Applied consistent function parameter formatting (2-space indentation with closing parenthesis on separate line)
  • Removed extraneous blank lines for cleaner code

Reviewed changes

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

File Description
R/filter_mzroll_list.R Core functionality change: added logical support to extract_ids_from_metadata() with new validate_filter_ids() helper; removed blank lines
R/mutate_mzroll_list.R Removed 2 blank lines for consistent formatting
R/import_mzroll.R Reformatted process_mzroll_multi() function parameters with consistent indentation
R/differential_expression.R Reformatted plot_volcano() function parameters with consistent indentation
Comments suppressed due to low confidence (1)

R/filter_mzroll_list.R:313

  • The new helper function validate_filter_ids lacks test coverage. Given that this function is critical for validating the type compatibility between filter_ids and the column values, and the codebase has test files for filters, consider adding tests to verify the behavior when both are logical, when there's a type mismatch, and when both are non-logical types.
validate_filter_ids <- function(col_values, filter_ids) {
  col_is_logical <- is.logical(col_values)
  ids_are_logical <- is.logical(filter_ids)
  if (col_is_logical && !ids_are_logical) {
    stop("\nfilter_var column is logical but filter_ids is not logical")
  }
  if (!col_is_logical && ids_are_logical) {
    stop("\nfilter_ids is logical but filter_var column is not logical")
  }
}

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

Comment thread R/filter_mzroll_list.R
checkmate::assert(
checkmate::checkCharacter(filter_ids),
checkmate::checkLogical(filter_ids, min.len = 1)
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not a big deal, but wondering why the nested asserts here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could be written in another way, but just checking that it is a character or logical

@PMSeitzer PMSeitzer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@johanna0321
johanna0321 merged commit e64d319 into main Feb 27, 2026
4 checks passed
@johanna0321
johanna0321 deleted the issue-48 branch February 27, 2026 20:11
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.

3 participants