Skip to content

enable return_linter - #725

Open
alanahjonas95 wants to merge 7 commits into
mainfrom
aj641247-gsk-fix-return-linter
Open

enable return_linter#725
alanahjonas95 wants to merge 7 commits into
mainfrom
aj641247-gsk-fix-return-linter

Conversation

@alanahjonas95

@alanahjonas95 alanahjonas95 commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

resolves #726

alanahjonas95 and others added 5 commits July 21, 2026 10:03
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@alanahjonas95
alanahjonas95 requested a review from dragosmg July 21, 2026 10:17
@alanahjonas95 alanahjonas95 self-assigned this Jul 21, 2026

@dragosmg dragosmg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Apologies for the repeated comment

Comment thread R/tfrmt.R Outdated
if (missing(arg_call)) {
## args not defined can quietly return empty expressions.
return(quote(expr = ))
quote(expr = )

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are early returns, I'd keep the return() call here. Do they trigger the linter?

@alanahjonas95 alanahjonas95 Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@dragosmg yeah all these that you've commented on trigger the linter, ive just changing this one for now, should i add nolint or do we not want to turn this lintr on as theres quite a lot?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

first thought would be to go with nolint where needed. I'll have a deeper look.

Comment thread R/col_plan.R
if (is.name(x)) {
if (identical(as_label(x), "<empty>")) {
return(NULL)
NULL

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are early returns, I'd keep the return() call here. Do they trigger the linter?

Comment thread R/col_plan.R
}
} else if (is.character(x)) {
return(as_length_one_quo.character(x))
as_length_one_quo.character(x)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are early returns, I'd keep the return() call here. Do they trigger the linter?

Comment thread R/col_plan.R
is_valid_quo_call(x) || is_valid_span_structure_call(x)
) {
return(eval_tidy(x))
eval_tidy(x)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are early returns, I'd keep the return() call here. Do they trigger the linter?

Comment thread R/col_plan.R
NULL
} else {
return(quo(!!x))
quo(!!x)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are early returns, I'd keep the return() call here. Do they trigger the linter?

Comment thread R/col_plan.R
}
} else if (is.character(x)) {
return(as_length_one_quo.character(x))
as_length_one_quo.character(x)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are early returns, I'd keep the return() call here. Do they trigger the linter?

Comment thread R/col_plan.R
quo(!!x)
} else if (is_valid_quo_call(x)) {
return(eval_tidy(x))
eval_tidy(x)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are early returns, I'd keep the return() call here. Do they trigger the linter?

Comment thread R/col_plan.R
NULL
} else {
return(quo(!!x))
quo(!!x)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are early returns, I'd keep the return() call here. Do they trigger the linter?

Comment thread R/col_plan.R
if (is.name(x)) {
if (identical(as_label(x), "<empty>")) {
return(NULL)
NULL

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are early returns, I'd keep the return() call here. Do they trigger the linter?

Comment thread R/tfrmt.R
}

return(arg_val)
arg_val

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are early returns, I'd keep the return() call here. Do they trigger the linter?

@github-actions

Copy link
Copy Markdown
Contributor

🦺 Coverage summary

badge

🟢 Merging PR #725 (7babe40) into main (ffaf8c9) will not change overall coverage.
🔴 Diff coverage is 80% (12 out of 15 modified lines are covered by tests). It's good practice to aim for at least 96.1% (the base branch test coverage).

♻️ Comment updated with the latest results.

Created on 2026-07-28 with covr2gh v0.0.0.9041.

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.

return_linter

2 participants