Skip to content

Conversation

@Kucharssim
Copy link
Member

@Kucharssim Kucharssim commented Nov 21, 2025

fixes SGC#6

I still need to write unit tests, but otherwise this is ready for review. @JTPetter do you know why the current unit tests are failing? It seems this happened recently (maybe some of the jaspTools updates or something?). Anyway, it would be great to fix that independently so that we make sure this PR doesn't break anything...

Regarding implementation, I am only wondering if Show Report option should also add info about the data transform, or whether it can stay separate (as it is now). I don't know exactly what is the use of the reporting functionality, so let me know.

@Kucharssim Kucharssim requested a review from JTPetter November 21, 2025 19:10
@JTPetter
Copy link
Contributor

No, I don't know either why the unit tests are failing. They work for me locally, but fail on GitHub. Maybe @vandenman has an idea? It indeed happened recently and is likely related to ggplot2 / jaspTools / jaspGraphs?

The use of the report functionality is to provide a single object with the most important results. So the transformation details can likely stay out for now.

@vandenman
Copy link
Contributor

The unit tests respect the lockfiles, so please update the lockfile as well and push it in the PR. To do that:

  1. Open the project in RStudio
  2. renv should automatically be activated
  3. run the tests/ ensure they pass
  4. call renv::snapshot() to snapshot the library state to the lockfile
  5. push the changes to the PR.

To update all packages, use renv::update(). jaspBase is somewhat broken atm but that should be good again in a few hours after this comment.

Co-authored-by: Julius Pfadt <[email protected]>
@JTPetter
Copy link
Contributor

JTPetter commented Dec 1, 2025

@vandenman Thank you!

@Kucharssim Can you fix the unit tests based on what Don laid out? (I understand this as something that needs to be done within each PR, right? If it's a general things that needs to be done once, I can of course do it in a separate PR)

@Kucharssim
Copy link
Member Author

Yes, sorry, I think it would be best to fix it in a separate PR. Do you have this covered?

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.

4 participants