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

Default trigger names are bit strange #165

Open
quevon24 opened this issue Sep 11, 2024 · 2 comments
Open

Default trigger names are bit strange #165

quevon24 opened this issue Sep 11, 2024 · 2 comments

Comments

@quevon24
Copy link
Contributor

quevon24 commented Sep 11, 2024

Hi, i don't know if this is a bug or if the intention is for it to be like this, but if you don't set a custom label for InsertEvent, UpdateEvent or DeleteEvent, you will get some weird trigger names because the label and the operation have the same name, something like:

  • pgtrigger_update_update_xxxxx
  • pgtrigger_delete_delete_xxxxx
  • pgtrigger_insert_insert_xxxxx

Maybe instead of generating the trigger name this way:

self.trigger_name = trigger_name or self.trigger_name or f"{self.label}_{self.operation}"

We can do something like:

self.trigger_name = trigger_name or self.trigger_name or f"{self.label}_{self.operation}" if self.label != self.operation else str(self.operation)

That way, if you don't pass a custom label you end up with something like:

  • pgtrigger_update_xxxxx
  • pgtrigger_delete_xxxxx
  • pgtrigger_insert_xxxxx
@wesleykendall
Copy link
Member

I remember seeing this as well a while ago, and I was hesitant to change it since it would result in many migrations being created. Some people that use pghistory for all models, for example, could have migrations generated for all of them at once.

so if it's changed, I think I'd:

  • only enable the behavior via a setting
  • Make it the default setting in an API break of a future version so that people aren't surprised by the new migrations

Hope that makes sense. I'm happy to tackle this as I agree that the naming is awkward

@quevon24
Copy link
Contributor Author

I remember seeing this as well a while ago, and I was hesitant to change it since it would result in many migrations being created. Some people that use pghistory for all models, for example, could have migrations generated for all of them at once.

so if it's changed, I think I'd:

  • only enable the behavior via a setting
  • Make it the default setting in an API break of a future version so that people aren't surprised by the new migrations

Hope that makes sense. I'm happy to tackle this as I agree that the naming is awkward

That makes sense, thanks for replying so quickly

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

No branches or pull requests

2 participants