Skip to content

Conversation

@l0lawrence
Copy link
Member

No description provided.

@l0lawrence l0lawrence marked this pull request as ready for review October 20, 2025 15:27
@Copilot Copilot AI review requested due to automatic review settings October 20, 2025 15:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the pylint check implementation to use a centralized virtual environment command execution pattern. The changes improve code maintainability by eliminating duplicate pip freeze logic and standardizing how commands are run within virtual environments.

Key changes:

  • Extracted pip freeze logic into a reusable pip_freeze() method in the base Check class
  • Replaced direct check_call() usage with the run_venv_command() method for better error handling
  • Added improved stdout/stderr output handling for both successful and failed pylint runs

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
eng/tools/azure-sdk-tools/azpysdk/pylint.py Refactored to use centralized pip freeze and venv command execution methods; improved error output handling
eng/tools/azure-sdk-tools/azpysdk/Check.py Added pip_freeze() method with uv pip support and imported get_pip_command function

cwd=package_dir,
check=True,
)
# todo: why???
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

The TODO comment 'why???' is unclear and doesn't explain what needs to be addressed. Either remove this comment if the code is correct, or clarify what question needs to be resolved (e.g., '# TODO: Determine if stdout/stderr printing is necessary or if logging would be more appropriate').

Suggested change
# todo: why???
# TODO: Determine if printing stdout/stderr is necessary or if logging would be more appropriate.

Copilot uses AI. Check for mistakes.
else:
command += ["freeze"]

result = subprocess.run(command, cwd=os.getcwd(), check=True, capture_output=True, text=True)
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

[nitpick] Using os.getcwd() as the current working directory is redundant since that's the default behavior of subprocess.run(). Consider removing the cwd parameter or using a more specific directory if needed for clarity.

Suggested change
result = subprocess.run(command, cwd=os.getcwd(), check=True, capture_output=True, text=True)
result = subprocess.run(command, check=True, capture_output=True, text=True)

Copilot uses AI. Check for mistakes.
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