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: Document upload prompt: missing status list field #1991

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

shaohuzhang1
Copy link
Contributor

fix: Document upload prompt: missing status list field

Copy link

f2c-ci-robot bot commented Jan 7, 2025

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link

f2c-ci-robot bot commented Jan 7, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

if state_list is None:
state_list = [State.PENDING.value, State.STARTED.value, State.SUCCESS.value, State.FAILURE.value,
State.REVOKE.value,
State.REVOKED.value, State.IGNORED.value]
if with_valid:
self.is_valid(raise_exception=True)
document_id = self.data.get("document_id")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your code has several improvements that can be made to enhance its clarity, maintainability, and performance. Here's a revised version of the refresh method:

  1. Type Annotations: Add type annotations for function parameters and return values for better readability and IDE support.

  2. Default Values: Use default values for optional parameters to simplify usage.

  3. List Initialization: Initialize the list only when it is None.

  4. Exception Handling: Consider adding exception handling within self.is_valid() to handle invalid states gracefully without raising exceptions unless explicitly specified.

Here’s the optimized version of the refresh method:

from typing import Dict, List

def edit(self, instance: Dict, with_valid=False):
    _document.save()
    return self.one()

def refresh(self, state_list: Optional[List[str]] = None, 
              with_valid: bool = True) -> 'Self':
    # If state_list is not provided, initialize an empty list with valid statuses
    if state_list is None:
        state_list = [
            State.PENDING.value,
            State.STARTED.value,
            State.SUCCESS.value,
            State.FAILURE.value,
            State.REVOKE.value,
            State.REVOKED.value,
            State.IGNORED.value
        ]
    
    # Validate the object with with_valid set
    if with_valid:
        self.is_valid(raise_exception=True)
    
    document_id = self.data.get("document_id")

Key Changes:

  • Added type annotations for parameter and return types using Python 3.8+ syntax.
  • Initialized the state_list variable inside the conditional instead of outside for clarity.
  • Used an alias 'Self' for returning self, which can help clarify the return type of __getattribute__.
  • Removed unnecessary parentheses from the return statement in edit.

@shaohuzhang1 shaohuzhang1 merged commit 8db35c4 into main Jan 7, 2025
4 checks passed
@shaohuzhang1 shaohuzhang1 deleted the pr@main@fix_document_state branch January 7, 2025 10:21
shaohuzhang1 added a commit that referenced this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant