Skip to content

Fix "partial argument match" warnings (#1977) #2046

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 3 commits into from
Oct 18, 2021
Merged

Fix "partial argument match" warnings (#1977) #2046

merged 3 commits into from
Oct 18, 2021

Conversation

bersbersbers
Copy link
Contributor

For the first version of this PR, I am most of all interested in if/how CI works.

I will then fix more of these warnings in later commits.

Finally, I wonder how tests should be adapted for this case. On my system, with options(warnPartialMatchArgs = TRUE) in .Rprofile, several existing tests are issuing warnings. One could use

options(warnPartialMatchArgs = TRUE, warnPartialMatchAttr = TRUE, warnPartialMatchDollar = TRUE)

in the setup of any test, but that might fail if dependencies issue such warnings.

@moutikabdessabour
Copy link
Contributor

moutikabdessabour commented Oct 13, 2021

@bersbersbers Thanks for the time you put on this. For the tests you can try calling using the code that was in the issue without loading the packages and use something like expect_warning(ggplotly(....), regexp = NA). And Only set the options inside the test scope. this looks good to me. I'm waiting for @cpsievert to give me the green light to merge.

test_that("a message describing the test", {
   # initialising the data and the plot p and set the options

  
   # the actual assertion
   expect_warning(ggplotly(p), regexp = NA)
})

@bersbersbers
Copy link
Contributor Author

@moutikabdessabour thanks for the explanation, that was very helpful. I also added a very short test now.

@moutikabdessabour
Copy link
Contributor

moutikabdessabour commented Oct 13, 2021

@bersbersbers can you please revert the changes to the other test files. Otherwise this looks good to me. Thanks again for your time.

@bersbersbers
Copy link
Contributor Author

@moutikabdessabour I did it. I think.

@moutikabdessabour
Copy link
Contributor

Great. I will wait until @cpsievert approves the PR and I will merge. Again thanks for your time.

Copy link
Collaborator

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

Please update NEWS.md accordingly

@bersbersbers
Copy link
Contributor Author

Please update NEWS.md accordingly

@cpsievert thanks - done!

@bersbersbers
Copy link
Contributor Author

@moutikabdessabour done :)

@moutikabdessabour
Copy link
Contributor

@bersbersbers can you please use rebase instead of the cmerge master commit". Once it's done I will merge the PR. Again thanks for your time and effort.

@bersbersbers
Copy link
Contributor Author

@moutikabdessabour I did it :)

@moutikabdessabour moutikabdessabour merged commit 4bb1e44 into plotly:master Oct 18, 2021
@moutikabdessabour
Copy link
Contributor

@bersbersbers I can't thank you enough for this.

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.

3 participants