search

rich-iannone / DiagrammeR

Graph and network visualization using tabular data in R
https://rich-iannone.github.io/DiagrammeR/
Other
1.7k stars 248 forks source link

Use chaining in checks + add snapshot tests #499

Closed olivroy closed 11 months ago

olivroy commented 11 months ago

This slows it down a little bit. (tests used to take around 72 s, now around 80 s)

The commit that speaks the most of the benefit of this is this one 1cc65f573bebb57232d73ed1bdd53ca28e6b0510

Let me know if you think this is useful and I will try to wrap it up (to avoid having too many types of error checks.

Use check_installed() for suggested packages handling.

Edit: Sorry for the size this PR has now...

I did a follow-up to #384 in 0d9dc6c

I did a follow-up to #341, #366 and #479 in 6cc8a34

I replace which(grepl()) by grep() in 7a2a19f

I use import-standalone-type-check.R to improve value checking.

I reduced the use of pipes in some occasions.

I use cli in favour of glue (not yet finalized)

I added some more && and || in

I am almost done. I finalized the checking to reduce the need for emit_error(), emit_warning() and emit_message() shortly.

How to review

rich-iannone commented 11 months ago

@olivroy this is phenomenally good! Thanks for doing this 👍

olivroy commented 11 months ago

Woo! Glad you like it. I will implement this next week or the one after in all instances. I will let you know once it is ready for review.

olivroy commented 11 months ago

Hi @rich-iannone,

This is ready for your review. Sorry, I got carried away and it got big. This PR aims to change nothing except error messages. Maybe you can start by looking at the changes in tests, which are minimal and are mainly to add snapshot tests.

This PR improves the consistency of the code generally. while improving error messages.

I just squeezed in a review of how conditions are checked. It seemed to have created a small speedup on my machine!

This is not complete. Some outstanding issues could be addressed in a follow-up. Finalize the replacement of emit_error(), emit_message() with cli for example

rich-iannone commented 11 months ago

@olivroy thank you so much for the hard work you put into this PR! This is one of those packages where lots of people are getting value despite a fair amount of neglect on my part. This is a great step towards modernizing the package and getting it set up for further development down the line.

This is all to say that I’m really grateful!

olivroy commented 11 months ago

@rich-iannone yes indeed. I will finalize the transfer of error messages / conditions to rlang / cli. Let me know if you see something that is not right. I can fix it.

Could you also re-run revdeps to make sure I didn't break anything? Seems unlikely since the package is well tested and all tests passed, but just to make sure! I made huge changes after all.