Skip to content

Conversation

MichaelChirico
Copy link
Collaborator

Part of #962

lint_message = function(expr) {
matched_call <- xml2::xml_text(xml2::xml_find_first(expr, "expr/SYMBOL_FUNCTION_CALL"))
lint_message <- sprintf("Don't use nested %s() calls; instead, try", matched_call)
paste(lint_message, "(1) data.table::fcase; (2) dplyr::case_when; or (3) merging to a lookup table.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

(3) may be better "using a lookup table". If the lookup is just ok one column, new[match(Col, old)] in general or unname(new[old]) for charactes is the optimal code in my testing.

Or maybe "merging or matching to a lookup table"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea. i like "using a lookup table". The merging approach is pretty ugly in general, if it's needed. The simple named_vector[[lookup_keys]] approach is nicer when it's possible.

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

Very nice. I'd consider this a candidate for default linters.
Also, common mistake? At least I've seen it many times in beginner code.
Happy to merge either way. LMK what you think.

@MichaelChirico
Copy link
Collaborator Author

my hesitation for adding it as a default is that the "good" alternatives might require new dependencies in general. the base-only alternatives can get pretty ugly... I'll leave it out of defaults for now. we can always revisit

@MichaelChirico MichaelChirico merged commit d6bacad into master Mar 25, 2022
@MichaelChirico MichaelChirico deleted the nested_ifelse branch March 25, 2022 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants