Skip to content

propagate cf-api errors #241

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

Merged
merged 3 commits into from
May 29, 2025
Merged

propagate cf-api errors #241

merged 3 commits into from
May 29, 2025

Conversation

KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented May 21, 2025

PR Type

Bug fix


Description

  • Remove custom 404 and 500 handlers

  • Rely on req.raise_for_status() for errors

  • Eliminate unused status code variables


Changes walkthrough 📝

Relevant files
Error handling
cfapi.py
Simplify error handling in get_blocklisted_functions         

codeflash/api/cfapi.py

  • Deleted not_found and internal_server_error variables
  • Removed special-case 404/500 conditional blocks
  • Now uses req.raise_for_status() for all HTTP errors
  • +0/-9     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing return on error

    The exception handler logs errors but doesn’t return a value, causing the function to return None instead of a dict. It should return an empty dict or appropriate default on failure.

    try:
        req = make_cfapi_request(endpoint="/verify-existing-optimizations", method="POST", payload=information)
        req.raise_for_status()
        content: dict[str, list[str]] = req.json()
    except Exception as e:
        logger.error(f"Error getting blocklisted functions: {e}")

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Convert lists to sets and return

    Convert the lists in content to sets and explicitly return the resulting dict. This
    aligns the output with the declared return type and ensures the function returns a
    value.

    codeflash/api/cfapi.py [186-187]

     req.raise_for_status()
     content: dict[str, list[str]] = req.json()
    +return {filename: set(funcs) for filename, funcs in content.items()}
    Suggestion importance[1-10]: 9

    __

    Why: The function never returns a value on success and the annotation expects set[str], so converting lists to sets and returning fixes both the missing return and the type mismatch.

    High
    Return empty dict on error

    Ensure the function always returns a dictionary on failure by adding a return
    statement in the exception block. This prevents it from implicitly returning None
    and breaking callers expecting a dict.

    codeflash/api/cfapi.py [188-189]

     except Exception as e:
         logger.error(f"Error getting blocklisted functions: {e}")
    +    return {}
    Suggestion importance[1-10]: 7

    __

    Why: Without a return {} in the exception block, the function may implicitly return None, breaking callers expecting a dict.

    Medium

    @KRRT7 KRRT7 force-pushed the propagate-cf-api-errors branch from 4b8a6f6 to 3588bac Compare May 21, 2025 20:31
    Copy link

    openhands-ai bot commented May 28, 2025

    Looks like there are a few issues preventing this PR from being merged!

    • GitHub Actions are failing:
      • end-to-end-test

    If you'd like me to help, just leave a comment, like

    @OpenHands please fix the failing actions on PR #241

    Feel free to include any additional details that might help me get this PR into a better state.

    You can manage your notification settings

    @KRRT7 KRRT7 merged commit 246801d into main May 29, 2025
    16 checks passed
    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.

    2 participants