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

refactor: add @final to contents, types, and forms #2033

Merged
merged 5 commits into from
Jan 3, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Dec 23, 2022

This type hint makes it clear (and, in future, enforces) that contents, types, and forms are all final non-inheritable types.

@codecov
Copy link

codecov bot commented Dec 23, 2022

Codecov Report

Merging #2033 (46ea4ec) into main (08a099a) will increase coverage by 0.08%.
The diff coverage is 98.48%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/typing.py 88.88% <75.00%> (-2.78%) ⬇️
src/awkward/contents/bitmaskedarray.py 68.02% <100.00%> (+0.10%) ⬆️
src/awkward/contents/bytemaskedarray.py 88.54% <100.00%> (+0.02%) ⬆️
src/awkward/contents/emptyarray.py 72.22% <100.00%> (+0.15%) ⬆️
src/awkward/contents/indexedarray.py 77.66% <100.00%> (+0.05%) ⬆️
src/awkward/contents/indexedoptionarray.py 88.67% <100.00%> (+0.01%) ⬆️
src/awkward/contents/listarray.py 90.45% <100.00%> (+0.01%) ⬆️
src/awkward/contents/listoffsetarray.py 80.20% <100.00%> (+0.02%) ⬆️
src/awkward/contents/numpyarray.py 91.05% <100.00%> (+0.01%) ⬆️
src/awkward/contents/recordarray.py 84.05% <100.00%> (+0.03%) ⬆️
... and 24 more

@agoose77 agoose77 temporarily deployed to docs-preview December 23, 2022 15:35 — with GitHub Actions Inactive
@jpivarski
Copy link
Member

A warning from PyLint (they easily get buried):

typing.final is not supported by all versions included in the py-version setting

Is there a way to import it as backports for Python versions as early as 3.7?

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.

That's right: the Content, Form, and Type subclasses should not be subclassed; they're final.

So are ak.types.ArrayType, ak.record.Record, and the ak.index.Index subclasses (e.g. ak.index.Index64). These are not subclasses of the above, but are closely related.

I suppose you can't annotate Content, Form, and Type (the superclasses) to say that they can't have subclasses beyond those that are defined in our codebase. (I think Scala has a way to say that: they have a lot of module-level/namespace-level type qualifiers.)

Also, this would only prevent people from defining subclasses and then type-checking them, right? We're not preventing people from hacking things as a test—just formally declaring something as "okay for production," right?

@agoose77
Copy link
Collaborator Author

agoose77 commented Dec 24, 2022

This is done, it just needs a fix for mypy on older Python versions. I think the issue is how we import typing_extensions and typing in awkward.typing. It seems like there's no easy way to say "import everything from typing and typing_extensions (if appropriate), where typing_extensions takes priority". I think I might have to explicitly import the appropriate types from typing_extensions, but I'll properly dig into this next week.

I suppose you can't annotate Content, Form, and Type (the superclasses) to say that they can't have subclasses beyond those that are defined in our codebase. (I think Scala has a way to say that: they have a lot of module-level/namespace-level type qualifiers.)

Unfortunately not, but I think we'll be OK at least with constraining the immediate subclasses.

Also, this would only prevent people from defining subclasses and then type-checking them, right? We're not preventing people from hacking things as a test—just formally declaring something as "okay for production," right?

Right, this is not runtime-enforced at all.

@agoose77 agoose77 force-pushed the agoose77/refactor-typing-final branch from 6e47ba3 to 46d477f Compare January 3, 2023 16:46
@agoose77 agoose77 temporarily deployed to docs-preview January 3, 2023 17:04 — with GitHub Actions Inactive
@agoose77 agoose77 enabled auto-merge (squash) January 3, 2023 21:09
@agoose77 agoose77 temporarily deployed to docs-preview January 3, 2023 21:15 — with GitHub Actions Inactive
@agoose77 agoose77 merged commit 92eab0b into main Jan 3, 2023
@agoose77 agoose77 deleted the agoose77/refactor-typing-final branch January 3, 2023 21:23
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.

None yet

2 participants