Skip to content

Conversation

@sinisaos
Copy link
Member

@sinisaos sinisaos commented Aug 4, 2025

Related to #1242

@sinisaos sinisaos requested a review from dantownsend August 4, 2025 04:46
@sinisaos sinisaos added the enhancement New feature or request label Aug 4, 2025
@dantownsend
Copy link
Member

This looks good - thank you. I'll try and give it a go tomorrow 👍

@sinisaos
Copy link
Member Author

sinisaos commented Aug 7, 2025

Great. Thanks.

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.

This looks good to me.

My only thoughts were:

  1. Whether we should be returning a 404 if a PUT tries to update a resource which doesn't exist (some people wouldn't treat this as an error). But this is the behaviour we already had in the previous templates, so lets leave it.
  2. We use Task._meta.primary_key. An alternative is to do Task.id, but then to add the following to the Task table definition:
class Task(Table):
    """
    An example table.
    """
    id: Serial
    name = Varchar()
    completed = Boolean(default=False)

@sinisaos
Copy link
Member Author

sinisaos commented Aug 9, 2025

  • Whether we should be returning a 404 if a PUT tries to update a resource which doesn't exist (some people wouldn't treat this as an error). But this is the behaviour we already had in the previous templates, so lets leave it.

I think that's the right behavior and using a callback method is really handy in those cases.

  • We use Task._meta.primary_key. An alternative is to do Task.id, but then to add the following to the Task table definition:

Usually when I'm not using a custom primary key, I let Piccolo to automatically create the primary key column. In that case Task._meta.primary_key is a convenient way to satisfy a tool like mypy. I think we should leave this, but if you want, I can change it.

@dantownsend dantownsend merged commit 5139ec0 into piccolo-orm:master Aug 9, 2025
46 checks passed
@dantownsend
Copy link
Member

Looks good, thanks!

@sinisaos sinisaos deleted the update_asgi_templates branch August 10, 2025 05:04
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