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

Miscellaneous improvements throughout the codebase #365

Merged
merged 105 commits into from
Aug 31, 2023
Merged

Miscellaneous improvements throughout the codebase #365

merged 105 commits into from
Aug 31, 2023

Conversation

trag1c
Copy link
Member

@trag1c trag1c commented Aug 25, 2023

Joint PR w/ @bswck :)

@bswck
Copy link
Member

bswck commented Aug 25, 2023

Summary:

  • Improved performance:
    • Optimized some calculations.
    • Introduced variables for objects that were suboptimally recreated instead of being reused.
    • Replaced == with is where applicable.
  • Used good practices:
    • Removed unnecessary () when raising exceptions without arguments.
    • Used pathlib instead of os.path where possible.
    • Used a list comprehension instead of a for+list.append() loop.
    • Reduced code repetition (where the repetition was unnecessary).
    • Replaced dict.keys() with dict when iterating over dictionary keys.
    • Used contextlib.suppress() in a try-except-pass scenario.
    • Used builder pattern to improve readability.
    • Replaced most one-use variables with drop-in expressions.
    • Replaced str.find(x) == 0 with str.startswith(x) and str.find(x) == -1 with x not in str.
    • Replaced return None with return in functions whose return value is None.
    • Used positive logic checking in conditional expressions/statements where it increases legibility.
  • Introduced naming changes in local variables—changing parameter names was avoided not to introduce breaking changes:
    • Corrected some names not to shadow identifiers from the built-in scope.
    • Made one-letter variable names in gen/list/dict comprehensions more verbose and meaningful.
  • Improved type hinting:
    • Corrected some type annotations to reflect the runtime reality.
    • Added some missing type annotations, shortened Literal[k] | Literal[j] to Literal[k, j].
  • Replaced X.__class__ with type(X) (there is no need to respect the descriptor behavior of __class__ for Event objects, can they ever be weakref proxies?).
  • Applied De Morgan's laws in conditional statements where applicable.
  • Corrected some docstrings to start with imperative mood.

Copy link
Member

@Secrus Secrus left a comment

Choose a reason for hiding this comment

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

Great work overall!

src/cleo/commands/base_command.py Outdated Show resolved Hide resolved
src/cleo/io/inputs/argv_input.py Show resolved Hide resolved
@trag1c trag1c requested a review from Secrus August 28, 2023 09:24
@Secrus Secrus changed the title Refactored the entire codebase Miscellaneous improvements throughout the codebase Aug 31, 2023
@Secrus Secrus added the skip news Skip the check for a news file label Aug 31, 2023
Yes, the performance is around the same
`Formatter.format_and_wrap()`
@Secrus Secrus merged commit e03ce6f into python-poetry:main Aug 31, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news Skip the check for a news file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants