Skip to content

Conversation

@sinisaos
Copy link
Member

Related to #918. Code written by @patrickneise (here) but never create PR. Thanks @patrickneise.

@sinisaos sinisaos added the enhancement New feature or request label Jul 30, 2025
Copy link
Member

@dantownsend dantownsend left a comment

Choose a reason for hiding this comment

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

Thanks for this - it looks good, just a few minor comments 👍

piccolo/table.py Outdated
email_columns.append(column)

if isinstance(column, Secret):
if isinstance(column, Secret) or column._meta.secret:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to check isinstance(column, Secret) - just column._meta.secret alone should work, because Secret columns should have column._meta.secret = True.

"""

class Classified(Table):
top_secret = Varchar(secret=True)
Copy link
Member

Choose a reason for hiding this comment

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

As a sanity check, add a non-secret column here too, to make sure only secret columns are added to _meta.secret_columns. For example:

class Classified(Table):
    top_secret = Varchar(secret=True)
    public = Varchar()

You might also consider merging this test with the above test, so we have this:

def test_secret_columns(self):
    """
    Make sure ``TableMeta.secret_columns`` are setup correctly.
    """

    class Classified(Table):
        top_secret = Secret()
        confidential = Varchar(secret=True)
        public = Varchar()

    self.assertEqual(
        Classified._meta.secret_columns,
        [Classified.top_secret, Classified.confidential]
    )

@dantownsend
Copy link
Member

This is good to go once the CI passes - thanks!

@dantownsend dantownsend merged commit b5e022a into piccolo-orm:master Aug 3, 2025
46 checks passed
@sinisaos sinisaos deleted the secret_columns_fix branch August 4, 2025 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants