Skip to content

Fixed Python Code #1789

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

Closed
wants to merge 5 commits into from
Closed

Fixed Python Code #1789

wants to merge 5 commits into from

Conversation

Siddhesh-Agarwal
Copy link

@Siddhesh-Agarwal Siddhesh-Agarwal commented Apr 17, 2023

Fixes: #1764 and #365

  • Reformatted the Python code (using black and isort)
  • Replace .format() method with f-strings
  • Used bitwise operator << instead of ** operator. For example 2**n can rewritten as 1<<n (resulting in operation time reducing by >40%)
  • Replaced OrderedDict with default dict
  • Added Type hinting using the built-in typing library
  • Added docstrings where ever necessary
  • Rewrote some condensed for-loops for increased readability
  • Fixed some typos

@CLAassistant
Copy link

CLAassistant commented Apr 17, 2023

CLA assistant check
All committers have signed the CLA.

@matttm
Copy link

matttm commented Apr 19, 2023

Does their server even support a version of python that supports f-strings? AWS Lambda only supports up to 3.9 of python, which does not have f-strings, for instance.

@Siddhesh-Agarwal
Copy link
Author

Siddhesh-Agarwal commented Apr 19, 2023

Does their server even support a version of python that supports f-strings? AWS Lambda only supports up to 3.9 of python, which does not have f-strings, for instance.

That's a brilliant question! I am sure the Python version they are using supports f-strings because some part of their code was already using f-strings (basically it was a mix of both f-strings and .format()).

Also f-string have been there since Python 3.6

@franz101
Copy link

I also would suggest splitting up the PRs

Copy link

@harshsingh32 harshsingh32 left a comment

Choose a reason for hiding this comment

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

Looks fine

@Siddhesh-Agarwal
Copy link
Author

@dzhao @pouriya @MrAuro I am tagging you to bring your attention to this PR. I have spent hours and improved most of the Python code in this repo and would like to know if it is possible to see this PR merged.

@Risae
Copy link

Risae commented Apr 27, 2023

I also would suggest splitting up the PRs

This is quite a big PR and i agree with him that you should probably split your PR in order for the Twitter devs to better implement your changes.

@Siddhesh-Agarwal
Copy link
Author

@Risae @franz101 Should I split the code folder-wise and create PRs?

@Risae
Copy link

Risae commented Apr 28, 2023

@Risae @franz101 Should I split the code folder-wise and create PRs?

That sounds like a good idea, should make it easy enough for the devs to review the code changes.

@FredipusRex
Copy link

@Risae @franz101 Should I split the code folder-wise and create PRs?

The way to split a PR like this up is in terms of structure:

  1. Have the code formatting changes as one PR - you are making no code changes, just white space
  2. Have the docstring changes as another PR - no code changes, just documentation
  3. Have the typos as another PR
  4. Have the f-string changes as another PR - you are making simple, easy to parse code changes
  5. Have the dict changes as another PR - this might take some thought as to whether the ordering was important
  6. All your other code changes (for loops, bit shifts) - these are simpler to validate

As PRs are approved and merged, you rebase the remaining PRs

@Siddhesh-Agarwal Siddhesh-Agarwal closed this by deleting the head repository Jan 19, 2024
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.

7 participants