Skip to content

New nested_ifelse_linter #996

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

Merged
merged 2 commits into from
Mar 25, 2022
Merged

New nested_ifelse_linter #996

merged 2 commits into from
Mar 25, 2022

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