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

chore: add types to most of the uproot.source module #996

Merged
merged 10 commits into from
Oct 18, 2023
Merged

Conversation

lobis
Copy link
Collaborator

@lobis lobis commented Oct 18, 2023

Add types to most of the sources module.

Only added types where I was sure of the type.

I left things such as urls, file paths, etc. untyped instead of typing them as str since user may pass a pathlib path or similar.

@lobis lobis mentioned this pull request Oct 18, 2023
@lobis lobis requested a review from jpivarski October 18, 2023 17:11
@lobis lobis marked this pull request as ready for review October 18, 2023 17:11
Copy link
Member

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

Great, thanks! You can merge this as-is. My suggested commits below are just taking the typing further, which is optional.

All of the int argument types are a little dubious: these arguments sometimes take np.int64 and similar, depending on where the integer came from, and that's technically not an int. For runtime type-checking, I use

isinstance(x, numbers.Integral)

which takes NumPy integers as well as Python int, but I think that filling up these files with : numbers.Integral and especially : list[(numbers.Integral, numbers.Integral)] would obscure it.

src/uproot/source/chunk.py Outdated Show resolved Hide resolved
src/uproot/source/chunk.py Outdated Show resolved Hide resolved
src/uproot/source/file.py Outdated Show resolved Hide resolved
src/uproot/source/http.py Outdated Show resolved Hide resolved
src/uproot/source/http.py Outdated Show resolved Hide resolved
src/uproot/source/http.py Outdated Show resolved Hide resolved
src/uproot/source/http.py Outdated Show resolved Hide resolved
Comment on lines +373 to +379
def handle_no_multipart(
self,
source: uproot.source.chunk.Source,
ranges: list[(int, int)],
futures,
results,
):
Copy link
Member

Choose a reason for hiding this comment

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

See handle_multipart for comments on futures and results.

@lobis lobis enabled auto-merge (squash) October 18, 2023 18:14
@lobis lobis merged commit 2f99815 into main Oct 18, 2023
19 checks passed
@lobis lobis deleted the source-typing branch October 18, 2023 18:16
@agoose77
Copy link
Collaborator

For posterity, I don't think mypy understands the integral type. Also, there's the os.PathLike interface for non-str/bytes types, so we could define that in a local types module.

lobis added a commit that referenced this pull request Oct 19, 2023
…split

* origin/fsspec-urlsplit:
  feat: add support for current RNTuple files (#962)
  chore: update pre-commit hooks (#993)
  chore: add types to most of the `uproot.source` module (#996)
  fix: make hist import optional in test_0965 (#994)
  docs: add GaetanLepage as a contributor for test (#995)
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.

3 participants