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

Materials for Range tutorial #483

Merged
merged 2 commits into from
Jan 8, 2024
Merged

Materials for Range tutorial #483

merged 2 commits into from
Jan 8, 2024

Conversation

gahjelle
Copy link
Contributor

@gahjelle gahjelle commented Jan 5, 2024

Where to put new files:

  • New files should go into a top-level subfolder, named after the article slug. For example: my-awesome-article

How to merge your changes:

  1. Make sure the CI code style tests all pass (+ run the automatic code formatter if necessary).
  2. Find an RP Team member on Slack and ask them to review & approve your PR.
  3. Once the PR has one positive ("approved") review, GitHub lets you merge the PR.
  4. 🎉

Copy link
Contributor

@bzaczynski bzaczynski left a comment

Choose a reason for hiding this comment

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

@gahjelle Overall, your code looks good to me. I only flagged a few minor edge cases that it would be nice to handle. Let me know what you think. Thanks!

Comment on lines 83 to 85
start: int
stop: int
step: int
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these attributes be annotated with the float | int type hint like in the FloatRange class?

Suggested change
start: int
stop: int
step: int
start: float | int
stop: float | int
step: float | int

Comment on lines 47 to 50
if any(
self.step > 0 and self.stop <= self.start,
self.step < 0 and self.stop >= self.start,
):
Copy link
Contributor

Choose a reason for hiding this comment

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

any() expects an iterable. Perhaps you're missing an extra pair of parentheses?

Suggested change
if any(
self.step > 0 and self.stop <= self.start,
self.step < 0 and self.stop >= self.start,
):
if any((
self.step > 0 and self.stop <= self.start,
self.step < 0 and self.stop >= self.start,
)):

Comment on lines 91 to 94
if any(
self.step > 0 and element >= self.stop,
self.step < 0 and element <= self.stop,
):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here:

Suggested change
if any(
self.step > 0 and element >= self.stop,
self.step < 0 and element <= self.stop,
):
if any((
self.step > 0 and element >= self.stop,
self.step < 0 and element <= self.stop,
)):

@dataclass
class FloatRange:
start: float | int
stop: float | int = None
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically float | int | None.

stop: int
step: int
_num_steps: int = field(default=0, init=False)

Copy link
Contributor

Choose a reason for hiding this comment

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

The iterator protocol requires that an iterator object implements the .__iter__() method as well:

Suggested change
def __iter__(self):
return self

Without it, you won't be able to loop over the iterator itself or pass it into a list():

>>> for value in _FloatRangeIterator(1, 3, 0.5):
...     print(value)
...
1.0
1.5
2.0
2.5

>>> it = _FloatRangeIterator(1, 3, 0.5)
>>> list(it)
[1.0, 1.5, 2.0, 2.5]

Comment on lines 92 to 93
self.step > 0 and element >= self.stop,
self.step < 0 and element <= self.stop,
Copy link
Contributor

Choose a reason for hiding this comment

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

When .step is equal to zero, the iteration will never stop. The original range() function rejects zero as the step, so you could perhaps add similar validation in the .__post_init__() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've emphasized in the docstring that _FloatRangeIterator is non-public and should only be instantiated through FloatRange which does the validation. I'll leave the foot-gun for those desperate to use it ...

if self.stop is None:
self.stop = self.start
self.start = 0

Copy link
Contributor

Choose a reason for hiding this comment

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

If step is zero, we should raise an error to handle edge cases:

if math.isclose(self.step, 0):
    raise ValueError("'step' must not be zero")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is checked by if not isinstance(self.step, float | int) or self.step == 0 below. I'm switching to isclose().

@gahjelle
Copy link
Contributor Author

gahjelle commented Jan 8, 2024

Thanks @bzaczynski for the detailed review. I've updated the code.

Copy link
Contributor

@bzaczynski bzaczynski left a comment

Choose a reason for hiding this comment

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

@gahjelle Thank you for the updates. They looks good to me 👍

@KateFinegan KateFinegan merged commit c8eb6df into master Jan 8, 2024
1 check passed
@KateFinegan KateFinegan deleted the gahjelle-range-update branch January 8, 2024 23:58
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