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

WIP: custom row-metadata (#260) #328

Closed
wants to merge 4 commits into from
Closed

WIP: custom row-metadata (#260) #328

wants to merge 4 commits into from

Conversation

nbenn
Copy link
Contributor

@nbenn nbenn commented Jun 4, 2021

Ad #260

@nbenn
Copy link
Contributor Author

nbenn commented Jun 4, 2021

Happy to add some tests as well, something along the lines of the examples I supplied?

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks, the code looks good. Could you please add a snapshot test?

rowid_width <- 0L
} else {
rowid_width <- get_cell_widths(rowid)
Copy link
Member

Choose a reason for hiding this comment

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

Should get_cell_widths(NULL) return 0L, to simplify this code?

return(NULL)
}

if (is_false(has_row_id)) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be picked up by the methods instead?

@krlmlr
Copy link
Member

krlmlr commented Dec 23, 2021

This has conflicts now, unfortunately.

@krlmlr
Copy link
Member

krlmlr commented May 10, 2022

Done in #550.

@krlmlr krlmlr closed this May 10, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants