Skip to content

Conversation

@kieranjmartin
Copy link

Resolves #514

Incorportating check_ard_structure into as_card proved to not be straight forwards as as_card is used throughout the code base to coerce objects into a card class that do not pass the checks in check_ard_structure.

For now at least I did the following

  1. check_ard_structure can either error or warn based on user input. I made a function to switch between these (I couldn't see any function which existed to do this already). Part of me wonders about allowing the user a bit more control over what it considers an error or a warning; they can switch off caring about the column order, and whether there is a methods entry, but maybe we could allow more in future. That said I think it's probably better as a binary switch for a change
  2. In as_card, there is now an argument for whether you want to check if a card follows this structure. At the moment this defaults to TRUE, but if you think this is too strong I could switch it to default FALSE. Doing that would be the most low impact change.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 15, 2025

✅ All contributors have signed the CLA
Posted by the CLA Assistant Lite bot.

@kieranjmartin
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

Melkiades and others added 10 commits December 16, 2025 17:22
Co-authored-by: Davide Garolini <dgarolini@gmail.com>
Signed-off-by: Kieran Martin <kieranjmartin@gmail.com>
Co-authored-by: Davide Garolini <dgarolini@gmail.com>
Signed-off-by: Kieran Martin <kieranjmartin@gmail.com>
Co-authored-by: Davide Garolini <dgarolini@gmail.com>
Signed-off-by: Kieran Martin <kieranjmartin@gmail.com>
Co-authored-by: Davide Garolini <dgarolini@gmail.com>
Signed-off-by: Kieran Martin <kieranjmartin@gmail.com>
Co-authored-by: Davide Garolini <dgarolini@gmail.com>
Signed-off-by: Kieran Martin <kieranjmartin@gmail.com>
Co-authored-by: Davide Garolini <dgarolini@gmail.com>
Signed-off-by: Kieran Martin <kieranjmartin@gmail.com>
Co-authored-by: Davide Garolini <dgarolini@gmail.com>
Signed-off-by: Kieran Martin <kieranjmartin@gmail.com>
kieranjmartin and others added 3 commits December 29, 2025 09:52
Co-authored-by: Davide Garolini <dgarolini@gmail.com>
Signed-off-by: Kieran Martin <kieranjmartin@gmail.com>
@ddsjoberg ddsjoberg requested a review from bzkrouse January 5, 2026 18:51
Copy link
Collaborator

@bzkrouse bzkrouse left a comment

Choose a reason for hiding this comment

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

looks great to me!

@ddsjoberg
Copy link
Collaborator

Thanks @kieranjmartin ! Can you add a news entry and we'll merge?

@kieranjmartin
Copy link
Author

Thanks @kieranjmartin ! Can you add a news entry and we'll merge?

Done :)

@ddsjoberg
Copy link
Collaborator

@kieranjmartin thank you for this PR! It's going to be super useful!

Before we merge, can you assess if we need to make updates in cardx as well, e.g. those as_card(check = FALSE) changes? If so, can we make those changes before we merge? Thank you!

@kieranjmartin
Copy link
Author

@ddsjoberg I was thinking about this a little; I had a look at reverse dependencies and I can see that both gtsummary and cardx are using as_card; the other packages which takes cards as a dependency don't appear to use it.

I can go through both and add a PR to update this, but I am wondering if maybe I should make check=FALSE the default instead? To an extent I think the way it is currently coded is more principled, but I'm aware that this is a backwards incompatible change; if anyone is using this in a non package context they may get stung by the change too. So putting FALSE as the default would be a friendlier way to approach this.

@ddsjoberg
Copy link
Collaborator

Thanks @kieranjmartin !

I agree and like that the default is check = TRUE. For most users, I think this is what they will want when they call the function. There was recently a user who reported an issue in their code, where a check = TRUE would have caught it.

If you can check cardx, I can make the updates in gtsummary?

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.

Feature Request: Handle list columns

4 participants