search

joaquinanguera / aceR

An R package for processing ACE data
MIT License
3 stars 14 forks source link

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

Closed arjunpur closed 3 weeks ago

arjunpur commented 1 month ago

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.

arjunpur commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/joaquinanguera/aceR/pull/52#issuecomment-2158924974, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEYKLO7ATDRRLYHGOISCR63ZGXO7ZAVCNFSM6AAAAABJAMYIR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJYHEZDIOJXGQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

monicathieu commented 1 month ago

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 commented 1 month ago

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

arjunpur commented 1 month ago

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

monicathieu commented 3 weeks ago

Hooray! Confirming merge ok!