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

FIX: dropping cached_properties functionality #279

Closed
wants to merge 1 commit into from

Conversation

Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented Nov 5, 2022

Pull request type

Please check the type of change your PR introduces:

  • Code maintenance (refactoring, formatting, renaming, tests)

Pull request checklist

  • Code base maintenance (refactoring, formatting, renaming):

    • Docs have been reviewed and added / updated if needed
    • Lint (black rocketpy) has passed locally and any fixes were made
    • All tests (pytest --runslow) have passed locally

What is the current behavior?

cached_properties were introduced by #249 , but apperantly this functionality is available only on python 3.8 or higher.
As Google Colaboratory currently applies python 3.7 as default, we think it's not a good time to drop python 3.7 support.

What is the new behavior?

Simple search and replace @cached_property --> @property @functools.lru_cache()

Does this introduce a breaking change?

  • Yes
  • No

Other information

This only happened cause the github tests were seted to run using python 3.8, which was fixed by #262

@Gui-FernandesBR
Copy link
Member Author

  • Google Colab worked as expected (use !pip install git+https://github.com/RocketPy-Team/RocketPy.git@maint/drop_cached_properties_use to reproduce);
  • Tests are running and passing locally,

I think we are ready for review !!

@Gui-FernandesBR Gui-FernandesBR linked an issue Nov 5, 2022 that may be closed by this pull request
@Gui-FernandesBR Gui-FernandesBR mentioned this pull request Nov 11, 2022
21 tasks
@giovaniceotto
Copy link
Member

Please elaborate on the differences between @cached_property and @functools.lru_cache.

What are the benefits of @cached_property?

@giovaniceotto
Copy link
Member

@Gui-FernandesBR, please see #283

@Gui-FernandesBR
Copy link
Member Author

#283 already seems better in my opinion. I am closing this PR but will keep the branch for a while. If needed, we can easily re-open.

Thank you @giovaniceotto for helping!

@Gui-FernandesBR Gui-FernandesBR deleted the maint/drop_cached_properties_use branch November 17, 2022 02:54
@Gui-FernandesBR Gui-FernandesBR removed this from the Release v1.0.0 milestone Jan 11, 2023
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.

What if we rearrange postProcess method?
2 participants