Skip to content

Conversation

@skrobchik
Copy link
Contributor

@skrobchik skrobchik commented Feb 10, 2022

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

  • 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?

When following tutorials, one will run a function which requires an optional dependency "netCDF4". Neither errors nor beginner tutorial communicates that it has to be installed.

What is the new behavior?

Functions that require netCDF4 will now tell that with a comprehensive error message to the user.

Does this introduce a breaking change?

  • Yes
  • No

@Gui-FernandesBR Gui-FernandesBR added Enhancement New feature or request, including adjustments in current codes Good first issue Good for newcomers labels Feb 10, 2022
@Gui-FernandesBR Gui-FernandesBR requested review from MrGribel and removed request for giovaniceotto February 10, 2022 04:10
@Gui-FernandesBR
Copy link
Member

Thanks skrobchik!

@MrGribel could you please take a look with me, review and then merge it if ready?

@giovaniceotto
Copy link
Member

I believe we would need to merge this PR into the develop branch, rather than the master branch. @MrGribel, do you agree?

@skrobchik
Copy link
Contributor Author

@giovaniceotto should I close this PR and open another one or is there a way to change the branch?

@lucasfourier
Copy link
Contributor

lucasfourier commented Feb 10, 2022

@skrobchik You can find an 'edit' button in the upper right corner of the page. If you click it, you will be able to change were you want to merge your commits into. Let me know if you are able to do it or not, ok?
image

@skrobchik skrobchik changed the base branch from master to develop February 10, 2022 20:24
@skrobchik
Copy link
Contributor Author

Thank you @lucasfourier

Copy link
Contributor

@MrGribel MrGribel left a comment

Choose a reason for hiding this comment

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

Thanks for this @skrobchik. We've had been planning on adding it to the default installation as an optional dependency or even migrating away from netCDF4 altogether, but this is as good as it gets for now.

Linter seems to be complaining but I think it's due to it not using the latest version of black during tests, so no worries.

@MrGribel MrGribel merged commit b115cc5 into RocketPy-Team:develop Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement New feature or request, including adjustments in current codes Good first issue Good for newcomers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Getting started running your first simulation doesn't tell that netCDF4 must be installed

5 participants