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

optimization for loops and refac to py3 #528

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

ArtemIsmagilov
Copy link
Contributor

  1. rewrite for loops on list comprehentions
  2. rewrite python2 class on python3

2. rewrite python2 class on python3
@unode
Copy link
Collaborator

unode commented Jul 19, 2024

Hi @ArtemIsmagilov thanks a lot for all the PRs and code contributions.
I'm going through them as time allows.

This one however includes some rewrites that use a different style from the one used elsewhere in the code.
Specifically the multi for/if list comprehensions.

Personally, I'm not a fan of this syntax due to readability (e.g. lines 76-87). I find it harder to reason given it partially inverts the order of reading, including a walrus operator in the middle which extends to the comprehension scope. At the same time I understand that it's a cleaner way to handle the deep/nested indentation.

As this is my personal point of view I would also like to invite @attzonko to share his thoughts on this.

@ArtemIsmagilov
Copy link
Contributor Author

Hi @unode , thank you very much for the code review. Don't worry about checking my PRs, I don't expect rushed reviews, check at your convenience. Regarding the list comprehension style, it’s up to you to decide. Readability, understanding and reliability of code for a developer are much more important than performance, in my humble opinion. It seems important to you what happened in this cycle. In this case, it will be better to leave your code unchanged as you suggested.

Copy link

codeclimate bot commented Jul 19, 2024

Code Climate has analyzed commit 109c55b and detected 0 issues on this pull request.

View more on Code Climate.

@attzonko attzonko merged commit b64b972 into attzonko:main Dec 5, 2024
8 checks passed
attzonko pushed a commit that referenced this pull request Dec 11, 2024
* 1. rewrite for loops on list comprehentions
2. rewrite python2 class on python3

* check correct reply on `is None`
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