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.

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

Refactor: Assertions + Modularize #173

Merged
merged 17 commits into from
Jul 24, 2024

Conversation

vincentarelbundock
Copy link
Collaborator

The goals of this PR are:

  1. Group all type-specific transformations and settings together in tinyplot.R. This will make it easier to insert new types.
  2. Offload complex type-specific transformations and settings to separate files and functions. For readability and maintainability.
  3. Break off some of the self-contained code to separate functions and files.
  • facet_layout()
  • draw_element()
  • Several others would be possible, but I don't have more time for now.
  1. Start using some assertions to check user input. I added a file called assertions.R that I use in tinytable. It basically re-implements checkmate package assertions without dependencies. We don't get much out of this now, but eventually this should give us:
  • More useful error messages for users
  • Ability to make more assumptions about data structure in the functions, therefore simplifying our codebase.

@grantmcdermott
Copy link
Owner

This looks great, thanks. I should be able to do a proper review this evening.

@grantmcdermott
Copy link
Owner

I just cloned this PR locally and am running into several test errors when I run R CMD check and/or tinytest::test_all(). I'm bit worried that I encounter this locally but nothing is getting picked up on our CI suite.... but for, right now, can you just check that this is running fine on your system?

Quick example of a failing test from inst/tinytest/test-density.R:

pkgload::load_all("~/Documents/Projects/tinyplot")
#> ℹ Loading tinyplot
mtcars$am = as.factor(mtcars$am)
with(mtcars, tinyplot(mpg, by = am, type = "density", legend = list(x = "bottom!")))
#> Error in legend(x = list, x = "bottom!", title = "am", lty = c("solid", : formal argument "x" matched by multiple actual arguments

Created on 2024-07-22 with reprex v2.1.0

@vincentarelbundock
Copy link
Collaborator Author

Right, your example fails for me locally too. Should be a simple issue with legend. Will look into it soon.

@grantmcdermott
Copy link
Owner

Right, your example fails for me locally too.

But it should fail as part of the GH Actions CI too, right?? Or am I just missing something?

(I was panicking that the workflow file isn't running any tests---maybe due to an env var mishap---but I just confirmed that it is via #174, which intentionally pushes a failing test. I'm honestly stumped why this PR isn't yielding a bunch of failed jobs in its current state.)

@vincentarelbundock
Copy link
Collaborator Author

vincentarelbundock commented Jul 23, 2024

FYI,

  1. The tests start failing again ---both locally and on CI--- when I replace exit_if_not() by if() exit_file(). Could this be related to exit_file / exit_if_not not working markvanderloo/tinytest#126 ?
  2. AFAICT, there are only 3 actually failing tests, all of which including "density" and "legend". Should be easy to debug.

@grantmcdermott
Copy link
Owner

The tests start failing again ---both locally and on CI--- when I replace exit_if_not() by if() exit_file(). Could this be related to exit_file / exit_if_not not working markvanderloo/tinytest#126 ?

Ah, good catch. That's a tricky bug. But I'm happy for you to implement the workaround that you suggested.

@vincentarelbundock

This comment was marked as resolved.

@vincentarelbundock
Copy link
Collaborator Author

@grantmcdermott forget about my last post. I made a stupid mistake. When fixed, and with your boxplot commit merged, everything passes as expected.

@grantmcdermott
Copy link
Owner

Excellent thanks. I'll take a look after dinner.

Copy link
Owner

@grantmcdermott grantmcdermott left a comment

Choose a reason for hiding this comment

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

This looks really great @vincentarelbundock.

I've dropped some (mostly minor) request changes and clarification questions. Could you take a stab at actioning these and let me know once the revision is ready? Thanks!

inst/tinytest/_tinysnapshot/hist_simple.svg Outdated Show resolved Hide resolved
inst/tinytest/helpers.R Outdated Show resolved Hide resolved
inst/tinytest/test-boxplot.R Outdated Show resolved Hide resolved
.github/workflows/R-CMD-check.yaml Outdated Show resolved Hide resolved
R/assertions.R Outdated Show resolved Hide resolved
R/draw_elements.R Outdated Show resolved Hide resolved
R/facet.R Outdated Show resolved Hide resolved
R/histogram.R Show resolved Hide resolved
R/tinyplot.R Outdated Show resolved Hide resolved
R/tinyplot.R Show resolved Hide resolved
@vincentarelbundock
Copy link
Collaborator Author

I think everything is resolved. Only discussion points are: copyleft issue (probably OK, I think) and warning on automagic type change to boxplot.

@grantmcdermott grantmcdermott merged commit a109ad1 into grantmcdermott:main Jul 24, 2024
3 checks passed
@grantmcdermott
Copy link
Owner

Thanks for all the hard work on this! The main script was becoming increasingly unwieldy. So I'm very grateful that you took on the hard work of modularizing everything. 🎉🚀

I'll add NEWS item with attribution once I get time this evening.

@vincentarelbundock vincentarelbundock deleted the assertions branch September 9, 2024 00:55
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.

2 participants