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

Feat: raise AttributeError for public Array attributes #1573

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Jul 29, 2022

Fixes #1511

As discussed in the issue, the new behaviour is:

  • existing public class attributes can be assigned to (though the attribute itself may not be writable, e.g. with a read-only data descriptor)
  • private attributes can be assigned to, and will never attempt to set a field
  • fields can never be set

We might have some hidden issues if users employ underscore-prefixed fields. Right now, setting these attributes will write to the ak.Array object. However, this shadowing is already accepted when we read from an attribute - class attributes take precedence over fields, just as they do in this PR for writing. Are we happy with this?

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #1573 (284fa23) into main (9e17f29) will decrease coverage by 0.01%.
The diff coverage is 35.13%.

Impacted Files Coverage Δ
src/awkward/_v2/_connect/cuda/__init__.py 0.00% <0.00%> (ø)
src/awkward/_v2/_connect/numexpr.py 88.40% <0.00%> (ø)
src/awkward/_v2/_connect/pyarrow.py 88.46% <0.00%> (ø)
src/awkward/_v2/contents/bytemaskedarray.py 88.82% <0.00%> (ø)
src/awkward/_v2/contents/indexedarray.py 73.83% <0.00%> (ø)
src/awkward/_v2/contents/indexedoptionarray.py 89.14% <0.00%> (ø)
src/awkward/_v2/contents/listoffsetarray.py 81.85% <0.00%> (ø)
src/awkward/_v2/contents/unionarray.py 86.27% <0.00%> (ø)
src/awkward/_v2/numba.py 93.47% <0.00%> (ø)
src/awkward/_v2/operations/ak_from_avro_file.py 66.66% <0.00%> (ø)
... and 6 more

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.

This is good; I'd like to merge it, but I'm going to update the branch to main (after #1576 is done) before doing so.

@jpivarski jpivarski enabled auto-merge (squash) August 9, 2022 22:48
@jpivarski jpivarski merged commit 29b893d into main Aug 9, 2022
@jpivarski jpivarski deleted the agoose77/fix-error-for-setattr branch August 9, 2022 23:54
@agoose77
Copy link
Collaborator Author

I might change ... in dir(type(self)) to hasattr(self.__class__, ...) for simplicity, but I'll do it at a later date.

@jpivarski
Copy link
Member

Good point. Or maybe hasattr(type(self)) because if there's a public function that reveals a dunder-attribute, I think it should be preferred.

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.

Raise warning on setattr for assigning arrays to records
2 participants