Skip to content

.pr_agent_auto_best_practices

Jump to bottom
mrT23 edited this page Jan 22, 2025 · 28 revisions

Pattern 1: Add null-safety checks and error handling for potential runtime errors, particularly when dealing with file operations, string manipulations, or missing configuration values.

Example code before:

content = file_content.strip() + "\n"
model = config.model_weak

Example code after:

content = (file_content or "").strip() + "\n"
model = getattr(config, 'model_weak', config.model)
Relevant past accepted suggestions:
Suggestion 1:

Add null-safety checks to prevent potential runtime errors when handling string operations

Consider handling empty strings and None values explicitly before applying string operations to prevent potential AttributeError exceptions.

pr_agent/algo/utils.py [591-592]

-original_file_content_str = original_file_content_str.rstrip() + "\n"
-new_file_content_str = new_file_content_str.rstrip() + "\n"
+original_file_content_str = (original_file_content_str or "").rstrip() + "\n"
+new_file_content_str = (new_file_content_str or "").rstrip() + "\n"
Suggestion 2:

Add null check before string operations to prevent potential runtime errors

Add error handling for the case when response is empty or None before attempting string operations. The current implementation could raise AttributeError if response is None.

pr_agent/tools/pr_update_changelog.py [115-117]

+if not response:
+    return ""
 response = response.strip()
 if response.startswith("```"):
     response_lines = response.splitlines()
Suggestion 3:

Add graceful fallback when configuration settings are missing

Add error handling for the case when the model configuration is missing. Currently, if the config doesn't have the required model setting, it will raise an unhandled attribute error.

pr_agent/algo/pr_processing.py [357-360]

 if model_type == ModelType.WEAK:
-    model = get_settings().config.model_week
+    model = getattr(get_settings().config, 'model_week', get_settings().config.model)
 else:
     model = get_settings().config.model
Suggestion 4:

Add error handling for file operations to improve robustness

Consider adding error handling when opening and reading files to gracefully handle potential IOErrors or other exceptions that may occur during file operations.

pr_agent/tools/pr_help_message.py [102-105]

 for file in md_files:
-    with open(file, 'r') as f:
-        file_path = str(file).replace(str(docs_path), '')
-        docs_prompt += f"==file name:==\n\n{file_path}\n\n==file content:==\n\n{f.read().strip()}\n=========\n\n"
+    try:
+        with open(file, 'r') as f:
+            file_path = str(file).relative_to(docs_path)
+            docs_prompt += f"==file name:==\n\n{file_path}\n\n==file content:==\n\n{f.read().strip()}\n=========\n\n"
+    except IOError as e:
+        get_logger().error(f"Error reading file {file}: {e}")

Pattern 2: Fix broken or incorrect documentation links and improve link accessibility by ensuring URLs point to the correct resources and are publicly accessible.

Example code before:

[Test Guide](./implement.md)

Example code after:

[Test Guide](./test.md)
Relevant past accepted suggestions:
Suggestion 1:

Fix incorrect documentation link that could lead users to wrong resource

💎 Test (/test) | generate tests for a selected component, based on the PR code changes | +| 💎 Test (/test) | generate tests for a selected component, based on the PR code changes |








**The link in the Test tool entry is incorrect - it points to 'implement.md' instead of 'test.md'. This could confuse users trying to access the test documentation.**

[docs/docs/tools/index.md [17]](https://github.com/Codium-ai/pr-agent/pull/1417/files#diff-b3166060d77806092068e98904c634685a3d382b36aef22cf86f505efd280c79R17-R17)

```diff
-| **💎 [Test (`/test`](./implement.md))**                                                  | generate tests for a selected component, based on the PR code changes                                                                      |
+| **💎 [Test (`/test`](./test.md))**                                                  | generate tests for a selected component, based on the PR code changes                                                                      |
Suggestion 2:

Fix broken documentation link that points to an inaccessible edit URL

The tagging bot link points to an edit URL which is not publicly accessible. Update it to point to the correct documentation or section.

README.md [107]

-|       | [Tagging bot](https://github.com/Codium-ai/pr-agent/edit/main/README.md#try-it-now)                                                                                             | ✅ |                    |                    |              |
+|       | [Tagging bot](https://github.com/Codium-ai/pr-agent#try-it-now)                                                                                             | ✅ |                    |                    |              |

Pattern 3: Extract repeated code patterns into separate functions and use constants for commonly used values to improve maintainability and reduce duplication.

Example code before:

if condition1:
  return JSONResponse(status_code=200, content={"message": "success"})
if condition2:
  return JSONResponse(status_code=200, content={"message": "success"})

Example code after:

SUCCESS_RESPONSE = JSONResponse(status_code=200, content={"message": "success"})

if condition1 or condition2:
  return SUCCESS_RESPONSE
Relevant past accepted suggestions:
Suggestion 1:

Extract capping and logging logic into a separate function

Consider extracting the logic for capping and logging the extra lines into a separate function to reduce code duplication.

pr_agent/algo/pr_processing.py [41-46]

-if PATCH_EXTRA_LINES_BEFORE > MAX_EXTRA_LINES:
-    PATCH_EXTRA_LINES_BEFORE = MAX_EXTRA_LINES
-    get_logger().warning(f"patch_extra_lines_before was {PATCH_EXTRA_LINES_BEFORE}, capping to {MAX_EXTRA_LINES}")
-if PATCH_EXTRA_LINES_AFTER > MAX_EXTRA_LINES:
-    PATCH_EXTRA_LINES_AFTER = MAX_EXTRA_LINES
-    get_logger().warning(f"patch_extra_lines_after was {PATCH_EXTRA_LINES_AFTER}, capping to {MAX_EXTRA_LINES}")
+PATCH_EXTRA_LINES_BEFORE = cap_and_log_extra_lines(PATCH_EXTRA_LINES_BEFORE, "before")
+PATCH_EXTRA_LINES_AFTER = cap_and_log_extra_lines(PATCH_EXTRA_LINES_AFTER, "after")
 
+def cap_and_log_extra_lines(value, direction):
+    if value > MAX_EXTRA_LINES:
+        get_logger().warning(f"patch_extra_lines_{direction} was {value}, capping to {MAX_EXTRA_LINES}")
+        return MAX_EXTRA_LINES
+    return value
+
Suggestion 2:

Refactor the mock method to use a dictionary for improved efficiency and maintainability

Consider using a more efficient data structure, such as a dictionary, for the mock_get_content_of_file method. This would improve the readability and maintainability of the code, especially as the number of test cases grows.

tests/unittest/test_bitbucket_provider.py [25-40]

 def mock_get_content_of_file(self, project_key, repository_slug, filename, at=None, markup=None):
-    if at == '9c1cffdd9f276074bfb6fb3b70fbee62d298b058':
-        return 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile\n'
-    elif at == '2a1165446bdf991caf114d01f7c88d84ae7399cf':
-        return 'file\nwith\nmultiple \nlines\nto\nemulate\na\nfake\nfile\n'
-    elif at == 'f617708826cdd0b40abb5245eda71630192a17e3':
-        return 'file\nwith\nmultiple \nlines\nto\nemulate\na\nreal\nfile\n'
-    elif at == 'cb68a3027d6dda065a7692ebf2c90bed1bcdec28':
-        return 'file\nwith\nsome\nchanges\nto\nemulate\na\nreal\nfile\n'
-    elif at == '1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b':
-        return 'file\nwith\nsome\nlines\nto\nemulate\na\nfake\ntest\n'
-    elif at == 'ae4eca7f222c96d396927d48ab7538e2ee13ca63':
-        return 'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile'
-    elif at == '548f8ba15abc30875a082156314426806c3f4d97':
-        return 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile'
-    return ''
+    content_map = {
+        '9c1cffdd9f276074bfb6fb3b70fbee62d298b058': 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile\n',
+        '2a1165446bdf991caf114d01f7c88d84ae7399cf': 'file\nwith\nmultiple \nlines\nto\nemulate\na\nfake\nfile\n',
+        'f617708826cdd0b40abb5245eda71630192a17e3': 'file\nwith\nmultiple \nlines\nto\nemulate\na\nreal\nfile\n',
+        'cb68a3027d6dda065a7692ebf2c90bed1bcdec28': 'file\nwith\nsome\nchanges\nto\nemulate\na\nreal\nfile\n',
+        '1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b': 'file\nwith\nsome\nlines\nto\nemulate\na\nfake\ntest\n',
+        'ae4eca7f222c96d396927d48ab7538e2ee13ca63': 'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile',
+        '548f8ba15abc30875a082156314426806c3f4d97': 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile'
+    }
+    return content_map.get(at, '')
Suggestion 3:

Extract draft MR skipping logic into a separate function to improve code organization

Consider extracting the logic for checking if a merge request is a draft and should be skipped into a separate function. This will improve code readability and reduce duplication.

pr_agent/servers/gitlab_webhook.py [133-135]

-if draft and skip_draft_mr:
-    get_logger().info(f"Skipping draft MR: {url}")
-    return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
+if should_skip_draft_mr(draft, skip_draft_mr, url):
+    return SUCCESS_RESPONSE
Suggestion 4:

Use a constant for repeated success responses to improve code maintainability

Consider using a constant for the 'success' message to avoid repetition and improve maintainability. Define a constant at the module level and use it in all return statements.

pr_agent/servers/gitlab_webhook.py [124]

-return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
+return SUCCESS_RESPONSE
Suggestion 5:

Extract repeated code into a separate function to reduce duplication

Consider extracting the repeated JSONResponse creation into a separate function to reduce code duplication and improve maintainability.

pr_agent/servers/gitlab_webhook.py [139-143]

+def create_success_response():
+    return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
+
 if should_skip_mr(draft, url):
-    return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
+    return create_success_response()
 
 get_logger().info(f"New merge request: {url}")
 await _perform_commands_gitlab("pr_commands", PRAgent(), url, log_context)

Pattern 4: Use more efficient data structures like sets and dictionaries instead of lists when implementing lookups or handling unique values.

Example code before:

files_to_exclude = ['file1.md', 'file2.txt']
if filename in files_to_exclude:

Example code after:

files_to_exclude = {'file1.md', 'file2.txt'}
if filename in files_to_exclude:
Relevant past accepted suggestions:
Suggestion 1:

Use a set for faster lookup of excluded files

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

pr_agent/tools/pr_help_message.py [90]

-files_to_exclude = ['EXAMPLE_BEST_PRACTICE.md', 'compression_strategy.md', '/docs/overview/index.md']
+files_to_exclude = {'EXAMPLE_BEST_PRACTICE.md', 'compression_strategy.md', '/docs/overview/index.md'}
Suggestion 2:

Refactor the mock method to use a dictionary for improved efficiency and maintainability

Consider using a more efficient data structure, such as a dictionary, for the mock_get_content_of_file method. This would improve the readability and maintainability of the code, especially as the number of test cases grows.

tests/unittest/test_bitbucket_provider.py [25-40]

 def mock_get_content_of_file(self, project_key, repository_slug, filename, at=None, markup=None):
-    if at == '9c1cffdd9f276074bfb6fb3b70fbee62d298b058':
-        return 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile\n'
-    elif at == '2a1165446bdf991caf114d01f7c88d84ae7399cf':
-        return 'file\nwith\nmultiple \nlines\nto\nemulate\na\nfake\nfile\n'
-    elif at == 'f617708826cdd0b40abb5245eda71630192a17e3':
-        return 'file\nwith\nmultiple \nlines\nto\nemulate\na\nreal\nfile\n'
-    elif at == 'cb68a3027d6dda065a7692ebf2c90bed1bcdec28':
-        return 'file\nwith\nsome\nchanges\nto\nemulate\na\nreal\nfile\n'
-    elif at == '1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b':
-        return 'file\nwith\nsome\nlines\nto\nemulate\na\nfake\ntest\n'
-    elif at == 'ae4eca7f222c96d396927d48ab7538e2ee13ca63':
-        return 'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile'
-    elif at == '548f8ba15abc30875a082156314426806c3f4d97':
-        return 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile'
-    return ''
+    content_map = {
+        '9c1cffdd9f276074bfb6fb3b70fbee62d298b058': 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile\n',
+        '2a1165446bdf991caf114d01f7c88d84ae7399cf': 'file\nwith\nmultiple \nlines\nto\nemulate\na\nfake\nfile\n',
+        'f617708826cdd0b40abb5245eda71630192a17e3': 'file\nwith\nmultiple \nlines\nto\nemulate\na\nreal\nfile\n',
+        'cb68a3027d6dda065a7692ebf2c90bed1bcdec28': 'file\nwith\nsome\nchanges\nto\nemulate\na\nreal\nfile\n',
+        '1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b': 'file\nwith\nsome\nlines\nto\nemulate\na\nfake\ntest\n',
+        'ae4eca7f222c96d396927d48ab7538e2ee13ca63': 'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile',
+        '548f8ba15abc30875a082156314426806c3f4d97': 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile'
+    }
+    return content_map.get(at, '')

Pattern 5: Add clear security warnings and detailed privacy information in documentation, especially regarding sensitive data handling and credential management.

Example code before:

# Configuration
Add your credentials to .env:
API_KEY=your_key
TOKEN=your_token

Example code after:

# Configuration
Add your credentials to .env:
API_KEY=your_key
TOKEN=your_token

⚠️ Security Warning: Never commit .env to version control. Add it to .gitignore.
Relevant past accepted suggestions:
Suggestion 1:

Add security warning about protecting sensitive credentials in environment files

Add a security warning about not committing the .env file to version control, and recommend adding it to .gitignore to prevent accidental exposure of sensitive credentials.

docs/docs/installation/locally.md [77-84]

 You can define the environment variables in a plain text file named `.env` with the following content:

CONFIG__GIT_PROVIDER="gitlab" GITLAB__URL="" GITLAB__PERSONAL_ACCESS_TOKEN="" OPENAI__KEY=""


+> ⚠️ **Security Warning**: Never commit the `.env` file to version control as it contains sensitive credentials. Add `.env` to your `.gitignore` file to prevent accidental exposure.
+
Suggestion 2:

Expand on the security and privacy aspects of the PR-Chat feature

Consider adding more details about the security and privacy aspects of the PR-Chat feature. This will help address potential concerns users might have about using the tool.

docs/docs/chrome-extension/index.md [19-20]

-The Chrome extension will not send any code to the server.
-To access code from private repositories, we will first validate the user's identity and permissions, then generate responses using the existing PR-Agent Pro integration.
+### Security and Privacy
 
+We take your code's security and privacy seriously:
+
+- The Chrome extension does not send any code to external servers.
+- For private repositories, we use secure authentication to validate the user's identity and permissions.
+- Responses are generated using the existing PR-Agent Pro integration, ensuring your code stays within your trusted environment.
+- All communication is encrypted and follows best practices for data protection.
+
+For more details on our security measures, please refer to our [Security Policy](link-to-security-policy).
+

[Auto-generated best practices - 2025-01-22]

Clone this wiki locally