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

Hotfix for issues 251 and 261 #262

Merged
merged 3 commits into from
Oct 24, 2022
Merged

Hotfix for issues 251 and 261 #262

merged 3 commits into from
Oct 24, 2022

Conversation

giovaniceotto
Copy link
Member

Pull request type

Please check the type of change your PR introduces:

  • Code base additions (bugfix, features)
  • Code maintenance (refactoring, formatting, renaming, tests)
  • ReadMe, Docs and GitHub maintenance
  • Other (please describe):

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • Code base additions (for bug fixes / features):

    • Tests for the changes have been added
    • 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?

See #205, #251 and #261.

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information

See Colab at: https://colab.research.google.com/drive/1viSLeMVux1aaxqRhXTQ6Ubid7qG0-wUi?usp=sharing

@giovaniceotto giovaniceotto self-assigned this Oct 22, 2022
@giovaniceotto giovaniceotto added the Bug Something isn't working label Oct 22, 2022
@giovaniceotto
Copy link
Member Author

@MrGribel, I am a bit worried about the new verison requirement for netCDF4. What do you think happens if someone has a version above 1.6.0 and tries to update rocketpy?

@MrGribel
Copy link
Contributor

Congrats on the fast response @giovaniceotto

Addressing your question, I've had netCDF4-1.6.1 installed on my local machine, and pip uninstalled it before installing the older netCDF4-1.5.8 when given the new requirements file, which I assume the behavior you intended.

It worries me a bit that we are introducing a dependency on an older version of a package though (I've had experiences with these getting removed at times from PyPI, for example). I believe a better long term fix would be to figure out what's going on inside the netCDF4 library. Perhaps keep the issue #205 open for the time being.

Besides that, the rest of the changes seem like pretty straightforward fixes. Thanks for that!

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Thank you again, sir!

@Gui-FernandesBR Gui-FernandesBR merged commit a9f9fab into master Oct 24, 2022
@Gui-FernandesBR Gui-FernandesBR deleted the hotfix-261-251 branch October 26, 2022 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants