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

Make Range instances somewhat immutable #142

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

quicklizard99
Copy link
Contributor

@quicklizard99 quicklizard99 commented Mar 5, 2024

This PR addresses two problems with Range.

  1. Attributes of Range instances can be set, bypassing validation and violating class invariants:

    >>> from xocto import ranges
    >>> r = ranges.Range(0, 1)
    >>> r.start = 2
    >>> r.boundaries = '😱'
  2. Additional attributes can be set on instances of classes derived from Range

    Range uses __slots__, which prevents creation of instance dictionaries. Classes derived from Range do not have __slots__ = (), which means that instance dictionaries are created:

    >>> from xocto import localtime
    >>> r = ranges.FiniteDatetimeRange(localtime.datetime(2000, 1, 1), localtime.datetime(2000, 1, 3))
    >>> r.some_other_thing = "I shouldn't be here"
    >>> r.__dict__
    {'some_other_thing': "I shouldn't be here"}

Fix these problems:

  • add __slots__ = () to classes that have Range as a base class (preventing creation of instance dicts),
  • add Range.__setattr__ to raise AttributeError, and
  • alter Range.__init__ to use the same object.__setattr__ trick as attrs to set attributes during initialisation.

Copy link

sentry-io bot commented Mar 5, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: xocto/ranges.py

Function Unhandled Issue
__init__ ValueError: Invalid boundaries for range /propert...
Event Count: 1.0k
__init__ ValueError: Invalid boundaries for range octoener...
Event Count: 551
__init__ ValueError: Invalid boundaries for range octoener...
Event Count: 164
__init__ ValueError: Invalid boundaries for range octoener...
Event Count: 84
__init__ ValueError: Invalid boundaries for range /meter-r...
Event Count: 16

Did you find this useful? React with a 👍 or 👎

@quicklizard99 quicklizard99 force-pushed the make-range-instances-immutable branch 3 times, most recently from c473001 to a6cf892 Compare March 6, 2024 09:35
Prior to this change, `Range` instances (because it declares
`__slots__`) did not have instance dict but classes derived
from `Range` did have instance dicts - a confusing mix of paradigms.

This change adds `__slots__ = ()` to classes that have `Range` as
a base class, and adds two tests to ensure that instances do not
have a `__dict__` attribute. It also adds an initialiser to
`HalfFiniteRange` to set `boundary` using the slot, rather than
trying to use an instance dict.
@quicklizard99 quicklizard99 force-pushed the make-range-instances-immutable branch from a6cf892 to bd702d0 Compare March 6, 2024 10:35
_is_left_exclusive: bool
_is_left_inclusive: bool
_is_right_exclusive: bool
_is_right_inclusive: bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These types were previously implicitly declared when __init__ set attributes.

start: T
boundaries = RangeBoundaries.INCLUSIVE_EXCLUSIVE
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting the default value using a class attribute does not play with __slots__. I have moved this default to __init__.

xocto/ranges.py Outdated
else:
raise AttributeError(
f"'{type(self).__name__}' object has no attribute '{name}'"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These exception messages consistent with those raised by collections.namedtuple.

@quicklizard99 quicklizard99 marked this pull request as ready for review March 6, 2024 10:51
xocto/ranges.py Outdated
Comment on lines 243 to 245
raise AttributeError(
f"'{type(self).__name__}' object has no attribute '{name}'"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps explicit is better than implicit, but I wonder if we could delegate this branch to super()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point. Change to use super

Copy link
Contributor

@jarshwah jarshwah left a comment

Choose a reason for hiding this comment

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

Good change, thanks for making it. As this is a potentially breaking change we'll need to at least bump the minor version when releasing.

Prior to this change, attributes of `Range` instances can be set,
bypassing validation and violating class invariants.

This change adds `Range.__setattr__`, which raises `AttributeError`,
and alters `__init__` to use the same `object.__setattr__` trick as
`attrs` to set attributes during initialisation.
@quicklizard99 quicklizard99 force-pushed the make-range-instances-immutable branch from bd702d0 to d093dec Compare March 7, 2024 09:22
@quicklizard99 quicklizard99 merged commit 955adcc into main Mar 7, 2024
1 check passed
@quicklizard99 quicklizard99 deleted the make-range-instances-immutable branch March 7, 2024 09:24
jarshwah added a commit that referenced this pull request Mar 12, 2024
…immutable"

This reverts commit 955adcc, reversing
changes made to 112d79e.
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