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

Tr/ignore titile adjustments #1201

Merged
merged 1 commit into from
Sep 7, 2024
Merged

Tr/ignore titile adjustments #1201

merged 1 commit into from
Sep 7, 2024

Conversation

mrT23
Copy link
Collaborator

@mrT23 mrT23 commented Sep 7, 2024

PR Type

Enhancement, Bug fix


Description

  • Consolidated PR ignore logic into separate functions (is_bot_user and should_process_pr_logic) for Bitbucket, GitHub, and GitLab
  • Implemented consistent PR/MR ignore logic across all platforms based on title, labels, source branches, and target branches
  • Updated webhook handlers to use new ignore logic functions, improving code organization and maintainability
  • Removed redundant and outdated code, including backwards compatibility checks and unnecessary logging
  • Updated documentation to reflect new ignore configuration options and usage
  • Modified default configuration in configuration.toml to include pre-defined ignore patterns for PR titles

Changes walkthrough 📝

Relevant files
Enhancement
bitbucket_app.py
Enhance Bitbucket PR processing with ignore logic               

pr_agent/servers/bitbucket_app.py

  • Added is_bot_user function to identify and skip bot users
  • Implemented should_process_pr_logic to ignore PRs based on title,
    source/target branches
  • Updated webhook handler to use new functions for improved processing
    logic
  • +48/-14 
    github_app.py
    Refactor GitHub PR ignore logic and enhance processing     

    pr_agent/servers/github_app.py

  • Refactored PR ignore logic into separate functions: is_bot_user and
    should_process_pr_logic
  • Updated handle_request to use new ignore logic functions
  • Removed redundant logging statements
  • +53/-38 
    gitlab_webhook.py
    Enhance GitLab MR processing with ignore logic                     

    pr_agent/servers/gitlab_webhook.py

  • Added is_bot_user function to identify and skip bot users
  • Implemented should_process_pr_logic to ignore MRs based on title,
    labels, source/target branches
  • Updated webhook handler to use new functions for improved processing
    logic
  • +47/-4   
    Documentation
    additional_configurations.md
    Update PR ignore configuration documentation                         

    docs/docs/usage-guide/additional_configurations.md

  • Updated documentation for ignoring automatic commands in PRs
  • Clarified usage of ignore_pr_title, ignore_pr_source_branches, and
    ignore_pr_target_branches
  • Removed outdated information about ignore_mr_labels
  • +8/-16   
    automations_and_usage.md
    Remove outdated PR ignore configuration info                         

    docs/docs/usage-guide/automations_and_usage.md

    • Removed outdated information about ignore_pr_titles parameter
    +0/-7     
    Configuration changes
    configuration.toml
    Update default PR ignore configuration                                     

    pr_agent/settings/configuration.toml

  • Updated default values for ignore_pr_title
  • Clarified comments for ignore-related configuration options
  • +5/-9     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🏅 Score: 85
    🧪 No relevant tests
    🔒 No security concerns identified
    🔀 Multiple PR themes

    Sub-PR theme: Implement common PR ignore logic

    Relevant files:

    • pr_agent/servers/bitbucket_app.py
    • pr_agent/servers/github_app.py
    • pr_agent/servers/gitlab_webhook.py

    Sub-PR theme: Update documentation and default configuration for PR ignore logic

    Relevant files:

    • docs/docs/usage-guide/additional_configurations.md
    • docs/docs/usage-guide/automations_and_usage.md
    • pr_agent/settings/configuration.toml

    ⚡ Key issues to review

    Code Duplication
    The is_bot_user and should_process_pr_logic functions are implemented separately for each platform (Bitbucket, GitHub, GitLab). Consider creating a common utility module to reduce code duplication and improve maintainability.

    Potential Bug
    The is_bot_user function returns False for bot users and True for non-bot users, which is the opposite of what the function name suggests. This might lead to confusion and potential bugs.

    @mrT23
    Copy link
    Collaborator Author

    mrT23 commented Sep 7, 2024

    /improve

    @mrT23
    Copy link
    Collaborator Author

    mrT23 commented Sep 7, 2024

    /improve

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Sep 7, 2024

    PR Code Suggestions ✨

    Latest suggestions up to 247d5e8

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Use more specific exception types in error handling

    Consider using a more specific exception type instead of catching all exceptions.
    This will make the error handling more precise and easier to debug.

    pr_agent/servers/github_app.py [290-292]

    -except Exception as e:
    -    get_logger().error(f"Failed 'should_process_pr_logic': {e}")
    +except (KeyError, AttributeError) as error:
    +    get_logger().error(f"Failed 'should_process_pr_logic': {error}")
     return True
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using specific exception types can improve error handling and debugging, making the code more robust and easier to maintain.

    7
    Performance
    Use a set instead of a list for faster lookup operations

    Consider using a set instead of a list for bot_indicators to improve lookup
    performance, especially if the list grows larger.

    pr_agent/servers/gitlab_webhook.py [82-85]

    -bot_indicators = ['codium', 'bot_', 'bot-', '_bot', '-bot']
    +bot_indicators = {'codium', 'bot_', 'bot-', '_bot', '-bot'}
     if any(indicator in sender_name for indicator in bot_indicators):
         get_logger().info(f"Skipping bot user: {sender_name}")
         return True
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a set instead of a list can improve performance for lookups, especially if the list of bot indicators grows larger.

    6
    Best practice
    Use more descriptive variable names in exception handling

    Consider using a more descriptive variable name instead of 'e' in the except blocks.
    This will make the code more readable and maintainable.

    pr_agent/servers/bitbucket_app.py [99-101]

    -except Exception as e:
    -    get_logger().error("Failed 'is_bot_user' logic: {e}")
    +except Exception as error:
    +    get_logger().error(f"Failed 'is_bot_user' logic: {error}")
     return False
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Using more descriptive variable names improves code readability, but it's a minor improvement that doesn't significantly impact functionality.

    5
    Maintainability
    Use more descriptive parameter names to improve code readability

    Consider using a more descriptive name for the data parameter in the
    should_process_pr_logic function to better reflect its content and purpose.

    pr_agent/servers/gitlab_webhook.py [91-95]

    -def should_process_pr_logic(data, title) -> bool:
    +def should_process_pr_logic(merge_request_data, title) -> bool:
         try:
             # logic to ignore MRs for titles, labels and source, target branches.
             ignore_mr_title = get_settings().get("CONFIG.IGNORE_PR_TITLE", [])
             ignore_mr_labels = get_settings().get("CONFIG.IGNORE_PR_LABELS", [])
     
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While using more descriptive parameter names can improve readability, the current name 'data' is not severely unclear, and the change is relatively minor.

    4

    Previous suggestions

    Suggestions up to commit 1c46ca4
    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Extract ignore condition checks into separate functions for better modularity

    Consider extracting the logic for checking ignore conditions into separate functions
    for better modularity and readability.

    pr_agent/servers/github_app.py [258-285]

    -if title:
    +def should_ignore_title(title):
         ignore_pr_title_re = get_settings().get("CONFIG.IGNORE_PR_TITLE", [])
         if not isinstance(ignore_pr_title_re, list):
             ignore_pr_title_re = [ignore_pr_title_re]
         if ignore_pr_title_re and any(re.search(regex, title) for regex in ignore_pr_title_re):
             get_logger().info(f"Ignoring PR with title '{title}' due to config.ignore_pr_title setting")
    -        return False
    +        return True
    +    return False
     
    -# logic to ignore PRs with specific labels or source branches or target branches.
    -ignore_pr_labels = get_settings().get("CONFIG.IGNORE_PR_LABELS", [])
    -if pr_labels and ignore_pr_labels:
    -    labels = [label['name'] for label in pr_labels]
    -    if any(label in ignore_pr_labels for label in labels):
    -        labels_str = ", ".join(labels)
    -        get_logger().info(f"Ignoring PR with labels '{labels_str}' due to config.ignore_pr_labels settings")
    -        return False
    +def should_ignore_labels(pr_labels):
    +    ignore_pr_labels = get_settings().get("CONFIG.IGNORE_PR_LABELS", [])
    +    if pr_labels and ignore_pr_labels:
    +        labels = [label['name'] for label in pr_labels]
    +        if any(label in ignore_pr_labels for label in labels):
    +            labels_str = ", ".join(labels)
    +            get_logger().info(f"Ignoring PR with labels '{labels_str}' due to config.ignore_pr_labels settings")
    +            return True
    +    return False
     
    -ignore_pr_source_branches = get_settings().get("CONFIG.IGNORE_PR_SOURCE_BRANCHES", [])
    -ignore_pr_target_branches = get_settings().get("CONFIG.IGNORE_PR_TARGET_BRANCHES", [])
    -if pull_request and (ignore_pr_source_branches or ignore_pr_target_branches):
    +def should_ignore_branches(source_branch, target_branch):
    +    ignore_pr_source_branches = get_settings().get("CONFIG.IGNORE_PR_SOURCE_BRANCHES", [])
    +    ignore_pr_target_branches = get_settings().get("CONFIG.IGNORE_PR_TARGET_BRANCHES", [])
         if any(re.search(regex, source_branch) for regex in ignore_pr_source_branches):
             get_logger().info(
                 f"Ignoring PR with source branch '{source_branch}' due to config.ignore_pr_source_branches settings")
    -        return False
    +        return True
         if any(re.search(regex, target_branch) for regex in ignore_pr_target_branches):
             get_logger().info(
                 f"Ignoring PR with target branch '{target_branch}' due to config.ignore_pr_target_branches settings")
    -        return False
    +        return True
    +    return False
     
    +if should_ignore_title(title) or should_ignore_labels(pr_labels) or should_ignore_branches(source_branch, target_branch):
    +    return False
    +
    Suggestion importance[1-10]: 8

    Why: This suggestion significantly improves code maintainability and readability by breaking down a complex function into smaller, more manageable pieces.

    8
    Enhancement
    Use more descriptive variable names in list comprehensions

    Consider using a more descriptive variable name instead of regex in the list
    comprehensions. For example, pattern would be more clear about the purpose of the
    variable.

    pr_agent/servers/bitbucket_app.py [116]

    -if ignore_pr_title_re and any(re.search(regex, title) for regex in ignore_pr_title_re):
    +if ignore_pr_title_re and any(re.search(pattern, title) for pattern in ignore_pr_title_re):
     
    Suggestion importance[1-10]: 5

    Why: The suggestion improves code readability by using a more descriptive variable name, but it's a minor enhancement.

    5
    Use more descriptive parameter names in functions

    Consider using a more descriptive name for the data parameter in the is_bot_user
    function, such as user_data, to better reflect its content and purpose.

    pr_agent/servers/gitlab_webhook.py [78-84]

    -def is_bot_user(data) -> bool:
    +def is_bot_user(user_data) -> bool:
         # logic to ignore bot users (unlike Github, no direct flag for bot users in gitlab)
    -    sender_name = data.get("user", {}).get("name", "unknown").lower()
    +    sender_name = user_data.get("user", {}).get("name", "unknown").lower()
         if 'codium' in sender_name or 'bot_' in sender_name or 'bot-' in sender_name or '_bot' in sender_name or '-bot' in sender_name:
             get_logger().info(f"Skipping bot user: {sender_name}")
             return True
         return False
     
    Suggestion importance[1-10]: 4

    Why: While the suggestion improves code clarity, it's a minor change that doesn't significantly impact functionality or maintainability.

    4
    ✅ Suggestions up to commit cdfe184
    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Refactor ignore conditions into a more modular structure using a dictionary of check functions

    Consider using a dictionary to store the different ignore conditions and their
    corresponding check functions. This would make the code more modular and easier to
    maintain.

    pr_agent/servers/gitlab_webhook.py [95-119]

    -if ignore_mr_source_branches:
    +def check_source_branch(data, ignore_list):
         source_branch = data['object_attributes'].get('source_branch')
    -    if any(re.search(regex, source_branch) for regex in ignore_mr_source_branches):
    -        get_logger().info(
    -            f"Ignoring MR with source branch '{source_branch}' due to gitlab.ignore_mr_source_branches settings")
    +    return any(re.search(regex, source_branch) for regex in ignore_list)
    +
    +def check_target_branch(data, ignore_list):
    +    target_branch = data['object_attributes'].get('target_branch')
    +    return any(re.search(regex, target_branch) for regex in ignore_list)
    +
    +def check_labels(data, ignore_list):
    +    labels = [label['title'] for label in data['object_attributes'].get('labels', [])]
    +    return any(label in ignore_list for label in labels)
    +
    +def check_title(title, ignore_list):
    +    return any(re.search(regex, title) for regex in ignore_list)
    +
    +ignore_conditions = {
    +    'source_branch': (ignore_mr_source_branches, check_source_branch),
    +    'target_branch': (ignore_mr_target_branches, check_target_branch),
    +    'labels': (ignore_mr_labels, check_labels),
    +    'title': (ignore_mr_title, check_title)
    +}
    +
    +for condition, (ignore_list, check_func) in ignore_conditions.items():
    +    if ignore_list and check_func(data if condition != 'title' else title, ignore_list):
    +        get_logger().info(f"Ignoring MR due to gitlab.ignore_mr_{condition} settings")
             return False
     
    -if ignore_mr_target_branches:
    -    target_branch = data['object_attributes'].get('target_branch')
    -    if any(re.search(regex, target_branch) for regex in ignore_mr_target_branches):
    -        get_logger().info(
    -            f"Ignoring MR with target branch '{target_branch}' due to gitlab.ignore_mr_target_branches settings")
    -        return False
    -
    -if ignore_mr_labels:
    -    labels = [label['title'] for label in data['object_attributes'].get('labels', [])]
    -    if any(label in ignore_mr_labels for label in labels):
    -        labels_str = ", ".join(labels)
    -        get_logger().info(f"Ignoring MR with labels '{labels_str}' due to gitlab.ignore_mr_labels settings")
    -        return False
    -
    -if ignore_mr_title:
    -    if any(re.search(regex, title) for regex in ignore_mr_title):
    -        get_logger().info(f"Ignoring MR with title '{title}' due to gitlab.ignore_mr_title settings")
    -        return False
    -
    Suggestion importance[1-10]: 8

    Why: This suggestion significantly improves code modularity, maintainability, and reduces repetition, making it easier to add or modify ignore conditions in the future.

    8
    Extract PR processing logic into a separate function for better organization

    Consider extracting the logic for checking if a PR should be processed into a
    separate function to improve code organization and reusability.

    pr_agent/servers/github_app.py [301-306]

    -# logic to ignore PRs opened by bot, PRs with specific titles, labels, source branches, or target branches
    -if is_bot_user(sender, sender_type):
    +def should_handle_request(sender, sender_type, action, body):
    +    if is_bot_user(sender, sender_type):
    +        return False
    +    if action != 'created' and 'check_run' not in body:
    +        return should_process_pr_logic(sender_type, sender, body)
    +    return True
    +
    +if not should_handle_request(sender, sender_type, action, body):
         return {}
    -if action != 'created' and 'check_run' not in body:
    -    if not should_process_pr_logic(sender_type, sender, body):
    -        return {}
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves code organization and reusability, making the code more maintainable and easier to understand.

    7
    ✅ Use a more pythonic approach for checking bot-related substrings in the sender's name

    Consider using a more pythonic approach by utilizing the any() function with a
    generator expression instead of multiple if statements for checking bot-related
    substrings in the sender's name.

    pr_agent/servers/gitlab_webhook.py [81]

    -if 'codium' in sender_name or 'bot_' in sender_name or 'bot-' in sender_name or '_bot' in sender_name or '-bot' in sender_name:
    +bot_indicators = ['codium', 'bot_', 'bot-', '_bot', '-bot']
    +if any(indicator in sender_name for indicator in bot_indicators):
     

    [Suggestion has been applied]

    Suggestion importance[1-10]: 5

    Why: The suggestion makes the code more concise and pythonic, but the improvement is relatively minor in terms of functionality and readability.

    5
    Use a more descriptive variable name in the list comprehension

    Consider using a more descriptive variable name instead of regex in the list
    comprehension. For example, pattern would be more clear about the purpose of the
    variable.

    pr_agent/servers/bitbucket_app.py [116]

    -if ignore_pr_title_re and any(re.search(regex, title) for regex in ignore_pr_title_re):
    +if ignore_pr_title_re and any(re.search(pattern, title) for pattern in ignore_pr_title_re):
     
    Suggestion importance[1-10]: 3

    Why: The suggestion improves code readability slightly, but the change is minor and doesn't significantly impact functionality or maintainability.

    3

    @mrT23 mrT23 force-pushed the tr/ignore_titile_adjustments branch from bb48d22 to 147a8e0 Compare September 7, 2024 08:26
    @mrT23 mrT23 merged commit 6150256 into main Sep 7, 2024
    2 checks passed
    @mrT23 mrT23 deleted the tr/ignore_titile_adjustments branch September 7, 2024 08:27
    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.

    3 participants