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

update wb_dims error #984

Merged
merged 3 commits into from
Apr 8, 2024
Merged

update wb_dims error #984

merged 3 commits into from
Apr 8, 2024

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Apr 5, 2024

Quick fix. Turns out I got here for the first time today! Will finalize next week if necessary. valid_args_nams didn't exist. oops

Edit : WIll add test, seems like lacking coverage here https://app.codecov.io/gh/JanMarvin/openxlsx2/pull/984/blob/R/utils.R#L307

@JanMarvin
Copy link
Owner

I know all too well how hard it is to cover all conditions, especially certain warning() or stop() conditions that you probably never encounter in real life, or at least not in local tests. And although I have my concerns about these 100% coverage projects, many strange things in my code (bugs or at least thinkos) have been discovered using coverage tests, and that's why I'm so eager to always write tests that cover new changes.

@olivroy olivroy marked this pull request as ready for review April 8, 2024 14:29
Copy link
Owner

@JanMarvin JanMarvin left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@olivroy olivroy merged commit 1194783 into main Apr 8, 2024
9 checks passed
@olivroy olivroy deleted the correct-error branch April 8, 2024 16:34
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.

2 participants