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

Review on breakpoint.black #9

Open
4 tasks
Quantum-0 opened this issue Jul 5, 2022 · 0 comments
Open
4 tasks

Review on breakpoint.black #9

Quantum-0 opened this issue Jul 5, 2022 · 0 comments
Assignees
Labels
invalid This doesn't seem right

Comments

@Quantum-0
Copy link
Owner

Quantum-0 commented Jul 5, 2022

Summary

Review link

  • R13 TypeVar not used on L9,15,40
  • R7 Redundant "else" on L41,42,43,44
  • L10 Modify in-place and return on L66
  • R1 Missing type hints on L66,71,74,77

R13 TypeVar not used

on L9,15,40

When you want to show that the same type is used in different places - use TypeVar instead of Any.

R7 Redundant "else"

on L41,42,43,44

There is a well-known pattern "if a: return one else: return two". In fact, the else keyword is redundant, because when first if condition evaluates to True, the function returns, otherwise control flow goes on. Removing else makes the code cleaner and easier to read.

  1. TypeError, not IndexError
  2. Moving type error handling to top is more readable

Suggested change:

if not isinstance(i, int):
    raise TypeError("Index must be int")

self._internal_list[i % len(self)] = value

L10 Modify in-place and return

on L66

There are functions that receive input and return output. There are functions that receive some object and modify it in-place. For example, reversed() vs list.reverse(), dataframe.apply() vs dataframe.apply(inplace=True). Doing both at the same time is confusing: when function returns something, it's not expected that it has any side effect, but here it does.

Maybe it was defined like that in a specification, but anyway it looks weird.

R1 Missing type hints

on L66,71,74,77

Type hints help humans and linters (like mypy) to understand what to expect "in" and "out" for a function. Not only it serves as a documentation for others (and you after some time, when the code is wiped from your "brain cache"), but also allows using automated tools to find type errors.

Missing return type hint

@Quantum-0 Quantum-0 added the invalid This doesn't seem right label Jul 5, 2022
@Quantum-0 Quantum-0 assigned Quantum-0 and unassigned Quantum-0 Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

1 participant