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 added for sorbet type strict. #11754

Merged
merged 5 commits into from
Mar 10, 2025

Conversation

randhircs
Copy link
Member

What are you trying to accomplish?

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@randhircs randhircs requested a review from a team as a code owner March 6, 2025 21:52
@randhircs randhircs requested a review from Copilot March 6, 2025 21:52
Copy link

@Copilot 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.

PR Overview

This pull request updates the typed level for the Python requirement parser file to use Sorbet’s strict mode and adds explicit type signatures to many methods.

  • Updated the Sorbet type directive from "true" to "strict".
  • Added and updated type signatures (sig) for various methods throughout the file.
  • Adjusted handling of nullable values using T.must and T.nilable.

Reviewed Changes

File Description
python/lib/dependabot/python/file_parser/python_requirement_parser.rb Modified type annotations and method signatures to support Sorbet strict mode

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

python/lib/dependabot/python/file_parser/python_requirement_parser.rb:142

  • This change removes memoization of the pyenv versions, causing the command to run on every invocation. Consider reverting to the memoization pattern (using ||=) to improve performance if caching is intended.
@pyenv_versions = T.let(run_command("pyenv install --list"), T.nilable(String))

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@randhircs randhircs requested a review from Copilot March 6, 2025 23:06

Choose a reason for hiding this comment

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

PR Overview

This PR changes the Sorbet typing level from "true" to "strict" and adds detailed T::Sig method signatures throughout the PythonRequirementParser to improve type safety. Additionally, it replaces direct content calls with T.must wrappers for better type checking.

  • Updated Sorbet typing directive
  • Added method signatures using T::Sig
  • Modified file content accesses to use T.must for type assurance

Reviewed Changes

File Description
python/lib/dependabot/python/file_parser/python_requirement_parser.rb Introduces Sorbet strict mode and adds method signatures; updates file content access with T.must calls

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

python/lib/dependabot/python/file_parser/python_requirement_parser.rb:156

  • Replacing the ||= memoization with a direct assignment causes a new PipCompileFileMatcher to be created on every call, which may not be intended. Consider using memoization to instantiate it only once.
@pip_compile_file_matcher = T.let(PipCompileFileMatcher.new(pip_compile_files), T.nilable(PipCompileFileMatcher))

Tip: Leave feedback on Copilot's review comments with the 👎 and 👍 buttons to help improve review quality. Learn more

@dependabot dependabot deleted a comment from Copilot bot Mar 6, 2025
@randhircs randhircs force-pushed the randhircs/dependabot-python-sorbettype-strict branch from a571a49 to 6927339 Compare March 6, 2025 23:22
Copy link
Contributor

@markhallen markhallen left a comment

Choose a reason for hiding this comment

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

I think we can improve these untyped sigils

@randhircs randhircs force-pushed the randhircs/dependabot-python-sorbettype-strict branch from 6927339 to 14e2cea Compare March 7, 2025 13:07
@randhircs randhircs requested a review from markhallen March 7, 2025 13:14
@JamieMagee JamieMagee added the sorbet 🍦 Relates to Sorbet types label Mar 7, 2025
@randhircs randhircs self-assigned this Mar 7, 2025
@randhircs randhircs requested a review from JamieMagee March 10, 2025 19:23
Copy link
Contributor

@markhallen markhallen left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks

@randhircs randhircs force-pushed the randhircs/dependabot-python-sorbettype-strict branch from 169c187 to 616b3c4 Compare March 10, 2025 21:21
@randhircs randhircs merged commit 8ef3430 into main Mar 10, 2025
64 of 66 checks passed
@randhircs randhircs deleted the randhircs/dependabot-python-sorbettype-strict branch March 10, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: python sorbet 🍦 Relates to Sorbet types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants