Skip to content

Network display name#251

Merged
Nospamas merged 13 commits intomasterfrom
network_display_name
Feb 25, 2026
Merged

Network display name#251
Nospamas merged 13 commits intomasterfrom
network_display_name

Conversation

@Nospamas
Copy link
Contributor

@Nospamas Nospamas commented Feb 11, 2026

Alternative approach to human readable identifiers within the network table this PR:

  • Adds a network_display_name column
  • Populates it with existing network_names
  • Adds a trigger to prevent the existing network_name column being updated (still allows inserts)
  • On insert, populates display name with network_name if it isn't provided

It retains the same changes as from network_key for:

  • Adding versionable ORM tables for testing without the new column
  • Re-adding the history table to maintain column order
  • Adjustments needed for tests to pass

@Nospamas Nospamas requested a review from QSparks February 24, 2026 18:35
Copy link
Contributor

@QSparks QSparks left a comment

Choose a reason for hiding this comment

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

LGTM!
Most of this PR was already covered in #250.
Only one comment on dropping a function.

Copy link
Contributor

@jameshiebert jameshiebert left a comment

Choose a reason for hiding this comment

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

Awesome, John! Thanks so much... this feels much better to me than the network_key version that preceded this (and had too much noise). Just a few small comments to consider, but otherwise it looks good to go.

def upgrade():
# Disable existing triggers before modifying table structure so that we don't accidentally track
# the intermediate states
disable_primary_table_triggers("meta_network")
Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent, this is so much cleaner than the o.g. version!

sa.Column(
"network_display_name",
sa.String(),
nullable=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this to be non nullable? What will break downstream if there's a NULL display name?

@Nospamas Nospamas merged commit d03b635 into master Feb 25, 2026
5 checks passed
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.

3 participants