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 summary() to separate compute and print #677

Merged
merged 6 commits into from
Jan 17, 2022
Merged

Refactor summary() to separate compute and print #677

merged 6 commits into from
Jan 17, 2022

Conversation

michaelquinn32
Copy link
Collaborator

This is intended to go after #676

It handles one of the refactorings mentioned in #667

@michaelquinn32 michaelquinn32 changed the base branch from master to develop January 1, 2022 01:57
@elinw
Copy link
Collaborator

elinw commented Jan 1, 2022

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
Copy link
Collaborator Author

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
Copy link
Collaborator

elinw commented Jan 9, 2022

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
Copy link
Collaborator Author

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

@elinw
Copy link
Collaborator

elinw commented Jan 10, 2022

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
Copy link
Collaborator Author

Thanks!

You can still set the summary rule width, but I'll fix the other issue related to table header width soon.
#684

@michaelquinn32
Copy link
Collaborator Author

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

@elinw
Copy link
Collaborator

elinw commented Jan 17, 2022

Sure

@elinw elinw merged commit 279fe93 into develop Jan 17, 2022
@elinw elinw deleted the render branch January 17, 2022 14:54
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