Put in a warning, and a nrow check on loading ACE data

[arjunpur]

I noticed a bug where in modules where there were only practice trials we end up erasing the entire data frame because of this filter. The result is that when we attempt any future operations on the empty dataframe, the load_ace_bulk function will error out.

This PR adds a warning and some nrow checks to verify that the tibble we're operating on has non-zero rows.

[monicathieu]

These changes look good overall! Thank you for catching that edge case.

I have a couple requests and one small suggested code change:

1. Code change: around line 175 of load-ace.R, consider directly returning dat inside that if (nrow(dat) == 0) statement to make explicit that no additional processing is called on dat if it is empty
2. Request part 1: Can you upload an example of an all-practice failing raw data file to inst/extdata/nexus? (assuming it is Nexus data, you can make a new folder at that path and add it there) You can then access it in tests using file.path(aceR_sample_data_path("nexus"), "file-name.csv").
3. Request part 2: In tests/testthat/test-load-csv.R, can you write a test in testthat syntax to expect that calling load_ace_file() on the all-practice data file will return a 0-rows dataframe, and throws the expected warning?
4. Request part 3: Can you write a test in tests/testthat/test-ace-pipeline.R (you can put it in where line 26 is now) that tests that calling load_ace_bulk() on the all-practice data file returns an appropriate aceR dataframe but with the data column empty?

Let me know once those tests are written and passing, or if you have other questions! (The example tests should guide you but mind you I wrote them a long while ago and the styling may be a little hacky, so don't follow them to a T.)

[arjunpur]

That sounds good - I can make those changes! Thanks @monicathieu. Do you know if Travis CI is set up on this repository? I tried accessing the link in CONTRIBUTING.md (+ changing the repo name) but I don't think anything runs.

[arjunpur]

Another question @monicathieu : It looks like you have some unmerged changes on the development branch (which is where you created inst/extdata/nexus) - should we merge those into master before I merge this PR in? or should we let them be?

[monicathieu]

To your questions:

1. Once upon a LONG time ago I think Travis CI was set up to run, but no longer... I need to put that in an enhancement milestone. So many quality-of-life improvements, so little time! For the time being, if you can run the testthat tests locally and confirm they pass, that'll do.
2. Let's leave the development branch unmerged for now, as it's got a bunch of other Nexus updates that (while mostly ready to go) are not 100% tested through yet. We can merge the contents of inst/extdata/nexus across branches later!

[rrflynt]

Hi Monica, Can you please take me off of this email chain? Thanks Reta Riedinger (Flynt) (208) 309-1286

…

On Mon, Jun 10, 2024 at 10:29 AM Monica Thieu ***@***.***> wrote: To your questions: 1. Once upon a LONG time ago I think Travis CI was set up to run, but no longer... I need to put that in an enhancement milestone. So many quality-of-life improvements, so little time! For the time being, if you can run the testthat tests locally and confirm they pass, that'll do. 2. Let's leave the development branch unmerged for now, as it's got a bunch of other Nexus updates that (while mostly ready to go) are not 100% tested through yet. We can merge the contents of inst/extdata/nexus across branches later! — Reply to this email directly, view it on GitHub <#52 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEYKLO7ATDRRLYHGOISCR63ZGXO7ZAVCNFSM6AAAAABJAMYIR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJYHEZDIOJXGQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[monicathieu]

Hi Reta @rrflynt ,

GitHub doesn't allow me to remove you from the repository, but you can stop receiving notifications about repository updates by going to the repository page https://github.com/joaquinanguera/aceR/ , clicking "Unwatch" in the top right-hand corner, and then choosing the notification setting you prefer.

[arjunpur]

Hi @monicathieu - I've addressed your comments here. Let me know what you think!

[monicathieu]

Reviewed changes look good overall, thank you for working on this! Will approve merging if you think my small line edits make sense (see inline comments).

[arjunpur]

Awesome thanks for the suggestions - committed them. Verified tests pass locally too

[monicathieu]

Hooray! Confirming merge ok!

[monicathieu]

Accepting!