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

MAINT: convert windrose ipywidgets and jsonpickle to optional dependencies #368

Merged
merged 10 commits into from
Jun 14, 2023

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

Please check the type of change your PR introduces:

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

Pull request checklist

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

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

These little guys are always imported, even though they are barely used during rocketpy experience.

What is the new behavior?

Following the timezonefinder example, I converted the libraries to optional dependencies.
Now the rocketpy library is ligther, faster and more consistent.
I also took care to update the corresponding documentation files.

Does this introduce a breaking change?

  • No

docs/user/requirements.rst Outdated Show resolved Hide resolved
docs/user/requirements.rst Outdated Show resolved Hide resolved
requirements_test.txt Outdated Show resolved Hide resolved
@MateusStano
Copy link
Member

MateusStano commented May 27, 2023

One last thing, the optional packages are still in the requirements.txt. They should be removed from there I think

Also, need to update requirements.rst

@Gui-FernandesBR
Copy link
Member Author

One last thing, the optional packages are still in the requirements.txt. They should be removed from there I think

Paraphrasing Giovani, I have to say: "Yes. The requirements.txt file is used for development only, it is not included in the packaged served through PyPI."

@Gui-FernandesBR
Copy link
Member Author

@MateusStano ready for a re-review!

@phmbressan @giovaniceotto , the old requirement_test.txt file was renamed to requirements-tests.txt, I would appreciate having an approvement from you too.

VSCode now understands it is a requirements file:
image

Copy link
Collaborator

@phmbressan phmbressan left a comment

Choose a reason for hiding this comment

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

I have no problem with the content of this pull request and I will approve it if first some things are taken into account:

  1. It might be troublesome requiring from the user to install individually the extras package as needed, since it not only requires the python and pip knowledge to do so, but also the recognition of a possible ImportError in code.

  2. I believe it would be better if features from PEP 508 are employed to separate a bit better the project dependencies:

    • For standard use pip install rocketpy;
    • For analysis use pip install rocketpy[envanalysis].
    • Usage description.

    For future use this can be expanded further in the future, e.g. pip install rocketpy[orbitsim]. That way we have more control that the necessary imports for a feature are indeed installed.

This might be out of scope for this pull request. If you think this is a good idea, we might create another one.

@Gui-FernandesBR
Copy link
Member Author

I have no problem with the content of this pull request and I will approve it if first some things are taken into account:

  1. It might be troublesome requiring from the user to install individually the extras package as needed, since it not only requires the python and pip knowledge to do so, but also the recognition of a possible ImportError in code.

  2. I believe it would be better if features from PEP 508 are employed to separate a bit better the project dependencies:

    • For standard use pip install rocketpy;
    • For analysis use pip install rocketpy[envanalysis].
    • Usage description.

    For future use this can be expanded further in the future, e.g. pip install rocketpy[orbitsim]. That way we have more control that the necessary imports for a feature are indeed installed.

This might be out of scope for this pull request. If you think this is a good idea, we might create another one.

Hey, @phmbressan , this was an incredible suggestion, I really appreciate it a lot.
I added [env_analysis] and [all] options within the last commit.
I believe it is running as expected/suggested now, therefore I'm asking you a re-review.

Something that I saw other people doing is creating an [tests] option too. This would make the requirements.txt and requirements-tests.txt no longer necessary here in our library.

rocketpy/EnvironmentAnalysis.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@Gui-FernandesBR
Copy link
Member Author

@MateusStano , the merging is blocked because I need your approve before, since you have requested changes in the past.

Please take a look to review/approve this one ;)

Copy link
Member

@MateusStano MateusStano left a comment

Choose a reason for hiding this comment

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

all good

@Gui-FernandesBR Gui-FernandesBR merged commit 51e10b2 into beta/v1.0.0 Jun 14, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the maint/reduce-dependencies branch June 14, 2023 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants