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

Use withr::with_options() in lost_tags_action.R #9

Closed
chartgerink opened this issue Jul 23, 2024 · 1 comment · Fixed by #26
Closed

Use withr::with_options() in lost_tags_action.R #9

chartgerink opened this issue Jul 23, 2024 · 1 comment · Fixed by #26
Labels
good first issue Good for newcomers

Comments

@chartgerink
Copy link
Member

I introduced a # nolint to ensure our checks pass in #8 - this issue is to ensure we track the work needed to not ignore the linter and provide a proper alternative, that complies with the linter.

This will mean replacing the options() calls with the functionality provided in withr to do this safely.

@chartgerink chartgerink added the good first issue Good for newcomers label Jul 23, 2024
chartgerink added a commit that referenced this issue Jul 23, 2024
chartgerink added a commit that referenced this issue Jul 23, 2024
@Bisaloo
Copy link
Member

Bisaloo commented Jul 25, 2024

I think it's one of the rare cases where # nolint is appropriate. withr will help to modify options temporarily, which is what we want in most cases but not here.

Here, we want to be able to change the options() for the entire duration of the session. This is an uncommon need hence why lintr warns against it.

An alternative to avoid the nolint comment would be to update the lintr settings, as done in linelist:

https://github.com/epiverse-trace/linelist/blob/9e49d071844626454eea9766f7eadaba12121907/.lintr#L13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants