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: typo in uproot.source.xrootd.XRootDSource name #990

Merged
merged 3 commits into from
Oct 13, 2023

Conversation

jpivarski
Copy link
Member

I scanned all of @lobis's PRs for "XRootDSource" and "source.root" and it only appeared in #971.

We need a test for it, though.

@lobis lobis changed the title Fix typo in uproot.source.xrootd.XRootDSource name fix: typo in uproot.source.xrootd.XRootDSource name Oct 13, 2023
@lobis
Copy link
Collaborator

lobis commented Oct 13, 2023

@jpivarski I added a test for it. uproot.open was not being tested against xrootd files (the source was being used directly) that is why this typo had been passing the tests. (Perhaps this test should be in another file though, I made a quick fix using pytest fixtures).

Copy link
Member Author

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Most of the Sources tests were very early, around test_0001_*.py through test_0005_*.py. They were probably written before uproot.open existed, and that's how this slipped through. I know there are uproot.open uses with HTTP in other parts of the test suite (testing things other than merely being able to open the file with HTTP).

But this test is a positive improvement: it's walking through the control flow that leads from a URL with root:// to loading the XRootDSource, and would have caught the bug. So for now, at least, I'll merge this and make Uproot 5.1.1.

I'm not allowed to approve this PR, probably because I started it. Oh well, I'll merge without approval.

@jpivarski jpivarski enabled auto-merge (squash) October 13, 2023 20:19
@jpivarski jpivarski merged commit 388395c into main Oct 13, 2023
@jpivarski jpivarski deleted the jpivarski/fix-typo-in-XRootDSource-name branch October 13, 2023 20:24
ikrommyd added a commit to ikrommyd/egamma-tnp that referenced this pull request Oct 13, 2023
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