Skip to content

perf: avoid redundant keyring lookups for repositories without credentials#10959

Open
radoering wants to merge 1 commit into
python-poetry:mainfrom
radoering:perf/keyring
Open

perf: avoid redundant keyring lookups for repositories without credentials#10959
radoering wants to merge 1 commit into
python-poetry:mainfrom
radoering:perf/keyring

Conversation

@radoering

Copy link
Copy Markdown
Member

_get_credentials_for_repository queries keyring so that it does not make sense to query keyring again if password is None.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • This change removes the fallback password-manager lookup when a repository config exists but returns credentials without a password; if there are scenarios where a repo is configured with only a username and relies on URL-based keyring entries, you may want to confirm that this behavior change is intentional or document that such setups are no longer supported.
  • The detailed inline comment about AuthenticatorRepositoryConfig.get_http_credentials()’s behavior tightly couples this method to that implementation; consider moving some of this explanation to a shared docstring or helper so it stays accurate if the credential lookup strategy evolves.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- This change removes the fallback password-manager lookup when a repository config exists but returns credentials without a password; if there are scenarios where a repo is configured with only a username and relies on URL-based keyring entries, you may want to confirm that this behavior change is intentional or document that such setups are no longer supported.
- The detailed inline comment about AuthenticatorRepositoryConfig.get_http_credentials()’s behavior tightly couples this method to that implementation; consider moving some of this explanation to a shared docstring or helper so it stays accurate if the credential lookup strategy evolves.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@radoering radoering marked this pull request as draft June 19, 2026 12:17
@radoering radoering marked this pull request as ready for review June 19, 2026 12:20

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • This refactor changes behavior for repositories whose configured credentials have a None password (no longer falling back to a URL-based password manager lookup); if that case is expected in the wild, consider adding an explicit test or comment confirming that dropping the fallback is intentional.
  • The new explanatory comment above _get_credentials_for_repository is quite dense; consider tightening it (e.g., focusing on the key point that _get_credentials_for_repository already covers all meaningful keyring lookups) to make the rationale easier to scan.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- This refactor changes behavior for repositories whose configured credentials have a `None` password (no longer falling back to a URL-based password manager lookup); if that case is expected in the wild, consider adding an explicit test or comment confirming that dropping the fallback is intentional.
- The new explanatory comment above `_get_credentials_for_repository` is quite dense; consider tightening it (e.g., focusing on the key point that `_get_credentials_for_repository` already covers all meaningful keyring lookups) to make the rationale easier to scan.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

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.

1 participant