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

fix: pushing new manifest without metadata update #907

Merged
merged 11 commits into from
Aug 11, 2024

Conversation

madams0013
Copy link
Contributor

When pushing an updated manifest to an existing prompt and not specifying any other options, update_prompt threw an error that we need to be updating something, but we're updating the manifest. This fixes.

@@ -5036,7 +5036,7 @@ def create_prompt(
description: Optional[str] = None,
readme: Optional[str] = None,
tags: Optional[Sequence[str]] = None,
is_public: bool = False,
is_public: Optional[bool] = False,
Copy link
Collaborator

@hinthornw hinthornw Aug 2, 2024

Choose a reason for hiding this comment

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

what's the need for this? / does "none" mean anything diff than false?

tags=tags,
is_public=is_public,
)
if not (
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: easier to read if we flip the conditions:

if (all this stuff is none) then create prompt, else update

Copy link
Contributor Author

@madams0013 madams0013 Aug 2, 2024

Choose a reason for hiding this comment

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

we still need to do this check after checking if prompt exists because if prompt doesnt' exist we just want to create it

if prompt exists AND none of these things are set, skip the call to update prompt

Copy link
Collaborator

@hinthornw hinthornw Aug 6, 2024

Choose a reason for hiding this comment

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

I'm just saying it's hard to read a double negative (not x is None vs. if x); the logic is right.

I find both of the following easier to read:

if any(
    parent_commit_hash is None
    and is_public is None
    and description is None
    and readme is None
    and tags is None
):
    self.create_prompt(
        prompt_identifier,
        is_public=is_public,
        description=description,
        readme=readme,
        tags=tags,
    )
else:
    self.update_prompt(
        prompt_identifier,
        description=description,
        readme=readme,
        tags=tags,
        is_public=is_public,
    )

or

if any (
    parent_commit_hash is not None
    and is_public is not None
    and description is not None
    and readme is not None
    and tags is not None
):
    self.update_prompt(
        prompt_identifier,
        description=description,
        readme=readme,
        tags=tags,
        is_public=is_public,
    )
else:
    self.create_prompt(
        prompt_identifier,
        is_public=is_public,
        description=description,
        readme=readme,
        tags=tags,
    )

Copy link
Collaborator

@hinthornw hinthornw left a comment

Choose a reason for hiding this comment

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

Awesome!

@madams0013 madams0013 merged commit 73aa21e into main Aug 11, 2024
8 checks passed
@madams0013 madams0013 deleted the maddy/patch-push-prompt-python branch August 11, 2024 18:29
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.

2 participants