Skip to content

minor improvements#23

Open
gafo25 wants to merge 4 commits intomasterfrom
refactor/minor_efficiency
Open

minor improvements#23
gafo25 wants to merge 4 commits intomasterfrom
refactor/minor_efficiency

Conversation

@gafo25
Copy link
Copy Markdown

@gafo25 gafo25 commented Apr 23, 2026

Changes

Minor changes have been made to improve efficiency. These changes will not affect current projects and are primarily related to refactoring.

R/phs_colours.R

  • First if conditional is the most used (when colourname is empty) It would be faster to avoid all the nested if else in function phs_colours
  • Function unname generates a copy in memory, It is more efficient to use names(result) <- NULL to remove names
  • If - else has been properly organised

R/phs_data.R

  • The list phs_palettes was calling the phs_colours function; however, we already have access to the phs_colour_values object. Using it should be faster because the function contains several conditional checks.

@gafo25 gafo25 requested review from Moohan and Tina815 April 23, 2026 12:22
Copy link
Copy Markdown
Member

@Moohan Moohan left a comment

Choose a reason for hiding this comment

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

Some comments and suggestions. You should also add yourself to the description at a contributor (role = "ctb")

Comment thread R/phs_colours.R Outdated
invalid_idx <- !colourname %in% names(phs_colour_values)
if (any(invalid_idx)) {
col_not_list <- colourname[invalid_idx]
stop("These colours are not available: ",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We tend to use {cli} for phs packages per tidyverse style, in this case cli::cli_abort(...

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Can you give me more details about it?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This suggestion may be considered in another PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment thread NEWS.md Outdated
Comment thread R/phs_colours.R Outdated
@gafo25 gafo25 requested a review from Moohan April 24, 2026 08:19
Comment thread R/phs_colours.R
paste(invalid_colours, collapse = ","),
"\nPlease run phs_colours() to see all the available colours"
)
stop(msg)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think {cli} will also need adding to imports usethis::use_package("cli")

Suggested change
stop(msg)
cli::cli_abort(c(
"Some colours are not available.",
x = "Unavailable colour{?s}: {.val {invalid_colours}}.",
i = "Run {.fn phs_colours} to see all available colours."
))

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.

2 participants