Skip to content

Rename keyring.py to keyring_utils.py#1765

Merged
yarikoptic merged 1 commit intomasterfrom
rename-keyring
Dec 11, 2025
Merged

Rename keyring.py to keyring_utils.py#1765
yarikoptic merged 1 commit intomasterfrom
rename-keyring

Conversation

@candleindark
Copy link
Copy Markdown
Member

This PR closes #1764.

Doing this avoids `keyring.py` shadowing the
`keyring` dependency pacakge.
@candleindark candleindark added the patch Increment the patch version when merged label Dec 5, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 5, 2025

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 75.03%. Comparing base (f5b8bc3) to head (d0b123b).
⚠️ Report is 95 commits behind head on master.

Files with missing lines Patch % Lines
dandi/dandiapi.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1765   +/-   ##
=======================================
  Coverage   75.03%   75.03%           
=======================================
  Files          84       84           
  Lines       11889    11889           
=======================================
  Hits         8921     8921           
  Misses       2968     2968           
Flag Coverage Δ
unittests 75.03% <88.88%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
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 renames the keyring.py module to keyring_utils.py to avoid naming conflicts with the external keyring package (as referenced in issue #1764).

Key Changes:

  • Module dandi/keyring.py renamed to dandi/keyring_utils.py
  • Import statements updated across the codebase
  • Test mock patch paths updated to reference the new module name

Reviewed changes

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

File Description
dandi/keyring_utils.py New file containing the renamed keyring utility functions (previously keyring.py)
dandi/dandiapi.py Updated import statement to reference keyring_utils instead of keyring
dandi/tests/test_keyring.py Updated import statement and all mock patch paths to reference keyring_utils module

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@candleindark candleindark marked this pull request as ready for review December 5, 2025 19:56
@yarikoptic
Copy link
Copy Markdown
Member

In general I don't mind, but to me it suggests the underlying tool likely trying to be too flexible with how it does imports etc, since it is infeasible to guarantee that we do not conflict with any potential python package installed on the system.

Although I don't expect but before we merge please search GitHub for dandi and keyring to ensure that no third party imports it.

@candleindark
Copy link
Copy Markdown
Member Author

@copilot Can you search GitHub to ensure no other packages import dandi.keyring as requested by @yarikoptic.

Copy link
Copy Markdown
Contributor

Copilot AI commented Dec 5, 2025

@candleindark I've opened a new pull request, #1766, to work on those changes. Once the pull request is ready, I'll request review from you.

@candleindark
Copy link
Copy Markdown
Member Author

candleindark commented Dec 5, 2025

Although I don't expect but before we merge please search GitHub for dandi and keyring to ensure that no third party imports it.

I think we are good to go. The only query that located some references of module is dandi.keyring NOT repo:dandi/dandi-cli. None of the matches is of concern. They are either in forks of dandi-cli or an auto-generated repo that doesn't actually import dandi.keyring.

See #1766 for detials.

@yarikoptic
Copy link
Copy Markdown
Member

were approvied, could've been merged (attn @candleindark )

@yarikoptic yarikoptic merged commit 3501190 into master Dec 11, 2025
43 of 44 checks passed
@yarikoptic yarikoptic deleted the rename-keyring branch December 11, 2025 12:07
@candleindark
Copy link
Copy Markdown
Member Author

candleindark commented Dec 11, 2025

were approvied, could've been merged (attn @candleindark )

Do you mean that I can merge any PR for this repo that is approved from now on? What about PRs for the dandischema repo?

@yarikoptic
Copy link
Copy Markdown
Member

yes, yes

@github-actions
Copy link
Copy Markdown

🚀 PR was released in 0.74.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch Increment the patch version when merged released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

keyring.py is shadows and create confusion with the keyring package

4 participants