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

Remove media arg #272

Merged
merged 35 commits into from
Sep 6, 2023
Merged

Remove media arg #272

merged 35 commits into from
Sep 6, 2023

Conversation

damianooldoni
Copy link
Member

Solving #255.

@damianooldoni damianooldoni changed the base branch from main to v1.0 September 4, 2023 12:00
@damianooldoni damianooldoni removed the request for review from PietrH September 4, 2023 12:01
@PietrH PietrH added this to the v1.0 milestone Sep 4, 2023
@damianooldoni damianooldoni marked this pull request as ready for review September 4, 2023 12:12
@damianooldoni damianooldoni requested review from PietrH and removed request for PietrH September 4, 2023 12:12
@damianooldoni
Copy link
Member Author

damianooldoni commented Sep 4, 2023 •
edited
Loading

Note to me: after PR #269 and #271 are merged to v1.0:

  1. Pull changes from v1.0 to my branch
  2. Add line to NEWS.md
  3. Set reviewer

Note also that the failure of R-CMD-check ubuntu-latest (3.5.0) should be solved by PR #269.

@damianooldoni
Copy link
Member Author

Note for me: after PR #271 has been merged to v1.0, please merge it to this branch and add a line in news.md.

@damianooldoni damianooldoni requested review from PietrH and peterdesmet and removed request for peterdesmet September 5, 2023 09:05
PietrH
PietrH requested changes Sep 6, 2023
View reviewed changes
tests/testthat/test-read_camtrap_dp.R Outdated Show resolved Hide resolved
tests/testthat/test-read_camtrap_dp.R Outdated Show resolved Hide resolved
tests/testthat/test-read_camtrap_dp.R Outdated Show resolved Hide resolved
tests/testthat/test-read_camtrap_dp.R Outdated Show resolved Hide resolved
tests/testthat/test-read_camtrap_dp.R Outdated Show resolved Hide resolved
tests/testthat/test-read_camtrap_dp.R Show resolved Hide resolved
tests/testthat/test-read_camtrap_dp.R Outdated Show resolved Hide resolved
tests/testthat/test-read_camtrap_dp.R Outdated Show resolved Hide resolved
tests/testthat/test-read_camtrap_dp.R Show resolved Hide resolved
tests/testthat/test-read_camtrap_dp.R Outdated Show resolved Hide resolved
@PietrH
Copy link
Member

PietrH commented Sep 6, 2023

@damianooldoni , I've made some suggestions, could you also look at bb04e3c? Using writeLines() results in the messages not being silenced by suppressMessages() .

In summary:

  • Consider using expect_type() and expect_s3_class() where possible
  • Consider using expect_named() where it makes sense
  • Lets try to reuse objects where possible, this way it's easier to make changes later, and it saves time running the tests

@damianooldoni @PietrH
use type and class expectations
9f0a53c 
Co-authored-by: Pieter Huybrechts <[email protected]>
@PietrH PietrH mentioned this pull request Sep 6, 2023
Merged
damianooldoni and others added 9 commits September 6, 2023 11:34
PietrH
PietrH approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@PietrH PietrH left a comment

Choose a reason for hiding this comment

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

Great job!

@damianooldoni damianooldoni merged commit 6d5519c into v1.0 Sep 6, 2023
8 checks passed
@damianooldoni damianooldoni deleted the remove-media-arg branch September 6, 2023 12:50
@damianooldoni damianooldoni mentioned this pull request Sep 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PietrH PietrH PietrH approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@damianooldoni @PietrH