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.

Sign up for GitHub

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

Jump to bottom

Refactoring of check_package() + return TRUE instead of package + tests #247

Merged
merged 43 commits into from
Jul 25, 2023
Merged

Refactoring of check_package() + return TRUE instead of package + tests #247

merged 43 commits into from
Jul 25, 2023

Conversation

PietrH
Copy link
Member

@PietrH PietrH commented Jul 14, 2023 •
edited
Loading

fixes #245

This PR is a blocker for #244

@PietrH PietrH linked an issue Jul 14, 2023 that may be closed by this pull request
Have check_package() return error or TRUE (on valid) #245
Closed
@PietrH PietrH changed the title 245 have check package return error or true on valid Refactoring of check_package() + return TRUE instead of package + tests Jul 14, 2023
PietrH added 11 commits July 14, 2023 14:56
@PietrH PietrH marked this pull request as ready for review July 14, 2023 14:53
@PietrH PietrH requested a review from damianooldoni July 14, 2023 14:54
@peterdesmet peterdesmet mentioned this pull request Jul 17, 2023
Open
12 tasks
@PietrH PietrH mentioned this pull request Jul 18, 2023
Merged
@damianooldoni
Merge main by resolving conflicts
edcccdf 
Especially taking care of improving check_package() in check_package.R based on the work done in #223
@damianooldoni
Adap read function to new output check_package
46ab17a
@damianooldoni
Avoid returning TRUE explicitly
15d971c
@damianooldoni damianooldoni mentioned this pull request Jul 24, 2023
Closed
@PietrH
Copy link
Member Author

PietrH commented Jul 25, 2023

Should the depreciation message be tested for every function individually? And if so, should it be tested as a snap (function wrapped in cat()), or with a testthat::expect_warning(call(), regexp = <regex>, fixed = TRUE) ?

I removed some of these checks as I felt they were redundant, and were causing me some trouble and as I felt they were covered elsewhere. But have no objection against testing things multiple times.

@damianooldoni
Copy link
Member

I thought to do so. Checks at function level should be self contained. The warning is returned by each function, so maybe good to test for each function separately. Notice that the warning will disappear soon as the arg datapkg will not be supported by version 1.0.

Agree to check better the warning by literal matching.

damianooldoni
damianooldoni approved these changes Jul 25, 2023
View reviewed changes
Copy link
Member

@damianooldoni damianooldoni left a comment

Choose a reason for hiding this comment

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

Nice job, @PietrH. I have just added some changes to deal with features introduced by other parallel PRs.

@PietrH PietrH merged commit ea86ddf into main Jul 25, 2023
@PietrH PietrH deleted the 245-have-check_package-return-error-or-true-on-valid branch July 25, 2023 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damianooldoni damianooldoni damianooldoni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Have check_package() return error or TRUE (on valid)
2 participants
@PietrH @damianooldoni