Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evaluate the necessity of the custom modify_defaults() function #16

Closed
Bisaloo opened this issue Jul 25, 2024 · 1 comment · Fixed by #27
Closed

Evaluate the necessity of the custom modify_defaults() function #16

Bisaloo opened this issue Jul 25, 2024 · 1 comment · Fixed by #27
Assignees

Comments

@Bisaloo
Copy link
Member

Bisaloo commented Jul 25, 2024

I don't exactly remember if/why this function was necessary.

I also think that modifyList() has been updated in the last couple of years and might now be appropriate to use directly.

We should evaluate if modify_defaults() is really necessary or can be replaced by modifyList().

In all cases, this function should have more & better comments:

@chartgerink
Copy link
Member

Thanks for this report 😊 Before I open the pull request where I address this, I document my evaluation as requested.

The primary code in modify_defaults that adds to the modifyList part is the following:

extra <- setdiff(names(x), names(defaults))
  if (strict && (length(extra) > 0L)) {
    stop(
      "Unknown variable types: ",
      toString(extra),
      "\n  ",
      "Use only known tags or set `allow_extra = TRUE`",
      call. = FALSE
    )
  }

This primarily is to check for known tags. Given that #22 indicates this can be dropped altogether, modify_defaults is indeed redundant. Will be removing this function and ensuring everything remains functional. 👍

@chartgerink chartgerink self-assigned this Aug 14, 2024
chartgerink added a commit that referenced this issue Aug 14, 2024
@chartgerink chartgerink linked a pull request Aug 14, 2024 that will close this 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 a pull request may close this issue.

2 participants