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

642 unused functions #659

Merged
merged 5 commits into from
Feb 21, 2024

Conversation

chlebowa
Copy link
Contributor

@chlebowa chlebowa commented Feb 21, 2024

Closes #642.

Removed substitute_q and extract_input.
Added @keywords internal to is_num_var_short.

@chlebowa chlebowa added the core label Feb 21, 2024
@chlebowa chlebowa linked an issue Feb 21, 2024 that may be closed by this pull request
@averissimo averissimo self-assigned this Feb 21, 2024
Copy link
Contributor

@averissimo averissimo left a comment

Choose a reason for hiding this comment

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

The removal of substitute_q and extract_input looks good.

I'm not sure about exporting create_sparklines until https://github.com/insightsengineering/coredev-tasks/issues/510 is closed

R/tm_variable_browser.R Show resolved Hide resolved
R/tm_variable_browser.R Outdated Show resolved Hide resolved
@m7pr m7pr mentioned this pull request Feb 21, 2024
26 tasks
Copy link
Contributor

@averissimo averissimo left a comment

Choose a reason for hiding this comment

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

LGTM

#' This might be retrieved like \code{teal.transform::data_extract_spec(...)[[1]]$dataname}.
#' @param filter logical if the connected \code{extract_data_spec} is used with \code{filter} option.
#' @keywords internal
extract_input <- function(varname, dataname, filter = FALSE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is a pure evil - it is clearly breaking encapsulation and introducing implementation details defined elsewhere in teal.transform. I disliked this from the very beginning. Happy to see it gone.

There is a clone of it in tmc but apparently it's used so not easy to remove...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I saw that. Since we're not conducting works there, I decided to put it off for later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you mind raising an issue to get rid of the evil over there? Maybe we can improve it or replace it altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will probably wait for bigger discussion about the future of teal.transform. Maybe we can overcome this in this way. Let's see.

@chlebowa chlebowa merged commit c57bc17 into pre-release@main Feb 21, 2024
@chlebowa chlebowa deleted the 642_unused_functions@pre-release@main branch February 21, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre-release activities - Unused functions
3 participants