search

ropensci / skimr

A frictionless, pipeable approach to dealing with summary statistics
https://docs.ropensci.org/skimr
1.1k stars 79 forks source link

Refactor summary() to separate compute and print #677

Closed michaelquinn32 closed 2 years ago

michaelquinn32 commented 2 years ago

This is intended to go after #676

It handles one of the refactorings mentioned in #667

elinw commented 2 years ago

Is the problem that led us to add https://github.com/ropensci/skimr/pull/677/files#diff-bc0bfdd73cb8cd74ba6bd991f16df8dbb07a2e4a3eeae24fd3a1a8180f75f479L56 fixed?

Also it's really helpful to return something that allows you to test if there is a result or NULL. Similar to always testing for a result versus error versus empty after a query. A lot of R code fails to do this and it causes problems later.

That said I really disliked the way it was working before so I'm happy to see the refactor!

michaelquinn32 commented 2 years ago

Since everything is being moved to pillar, and we are no longer inspecting the formatted string to remove metadata, it should be safe to enable crayon again. FWIW, I confirmed that it's working correcting on my windows 11 desktop (with colorized output).

elinw commented 2 years ago

I'm going to try this on my mac and also on the server. Did you see my width comment? @michaelquinn32

P.S. Really like the pipeable summary! It makes much more sense and is more like the normal summary.

michaelquinn32 commented 2 years ago

Sorry, I didn't see a comment on width. Do you mind reminding me? Thanks!

elinw commented 2 years ago

We had added the rule width parameter (that controls the horizontal line at the top of the printed skims) in response to a user request shown in the linked issue. I think we should still allow control of that.

michaelquinn32 commented 2 years ago

Thanks!

You can still set the summary rule width, but I'll fix the other issue related to table header width soon. https://github.com/ropensci/skimr/issues/684

michaelquinn32 commented 2 years ago

Can we merge and then I'll take on the issue with the header width soon?

elinw commented 2 years ago

Sure