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

refactor: convert for loops into list and dict comprehensions #2493

Merged
merged 14 commits into from
Sep 23, 2022
Merged

refactor: convert for loops into list and dict comprehensions #2493

merged 14 commits into from
Sep 23, 2022

Conversation

Uzaaft
Copy link
Contributor

@Uzaaft Uzaaft commented Sep 4, 2022

Description

The aim of this PR is to refactor all possible for-loops where lists and dicts are created into list-comprehensions, to get an easy gain of performance.

How has this been tested?

The list-comprehensions does not add any additional logic, it just required a rewrite of the old logic. If tests passed before, they will(and does locally), pass.

  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.

Checklist:

None of these are required since this is just a refactor

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My code passes all the checks pylint, flake8, black, ... To speed up development you should run pre-commit install.
  • New and existing unit tests pass locally with my changes. You can test this locally using pytest tests/....

for column in datasets.columns:
new_datasets[column] = datasets[column]

new_datasets = {column: datasets[column] for column in datasets.columns}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding this:
Why is this here at all? This causes the flake8 lint to fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable new_datasets is not being used later on after the if-test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DidierRLopes Do you know?
I'm inclined towards moving the unused variable. There seems to be no usage of it in the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was done by @JerBouma, probably he can shed some insight here

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 think a lot of these will come forward as I open PR's for the different refactors btw. 😅

Copy link
Contributor Author

@Uzaaft Uzaaft Sep 13, 2022

Choose a reason for hiding this comment

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

Thanks a lot @JerBouma!
@DidierRLopes Perhaps, going forward, if a variable is unused, should we just go ahead and remove it? Unless anything else is explicitly mentioned on the code?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Uzaaft yes, definitely unused variables should not be in the code. I wonder how that one passed the linters.

Would you be up for jumping into a call on Discord or elsewhere to coordinate the contributing efforts? This would make it easier and quicker to merge PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure thing @piiq.
After the merging that you did, I saw a few more possibilities of using list-comprehensions, so I am including those as well now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Uzaaft can you add me on discord please? pll_llq#8920

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 just sent you an request @piiq

@piiq piiq added the refactor Refactor code label Sep 23, 2022
@Uzaaft
Copy link
Contributor Author

Uzaaft commented Sep 23, 2022

@piiq All checks passed , but I need to update the branch once again😂

Copy link
Contributor

@piiq piiq left a comment

Choose a reason for hiding this comment

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

🚀

@piiq piiq merged commit 35b21ce into OpenBB-finance:main Sep 23, 2022
@Uzaaft
Copy link
Contributor Author

Uzaaft commented Sep 23, 2022

On to the Next one🤓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants