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: deprecate some methods before v1 #412

Merged
merged 4 commits into from
Sep 23, 2023

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

Please check the type of change your PR introduces:

  • [*] Code base additions (bugfix, features) : deprecations

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?

There are some functions in rocketpy that either does not work correctly or are already deprecated.

What is the new behavior?

I tried to search in the code for any function or method that we may need to deprecate in the short/medium term future.
I also converted some Environment methods to static methods, avoiding future breaking changes.

Does this introduce a breaking change?

  • Yes

Other information

it is not an easy task to imagine if a function would be deprecated in the future or not, but I tried my best here.
If reviewers find time, I also recommend reading all the classes to ensure there's nothing that we could easily modify right now.

** The idea is to prevent for future breaking changes.**

- the problem was happening when the angle was negative.
- i.e. the operator // didn't work
- I added some unit tests to cover the problematic cases

Everything running properly now
@Gui-FernandesBR
Copy link
Member Author

To discuss before review:

  • The Environment Class has these methods called all_plot_info_returned and all_info_returned which is meant to help with the API integration
  • Well, I agree it's important, but I have my questions if this is in the right place.
  • Maybe we should have a separate file to do the same operation for all the main classes. Just like the utilities.py does: receive a rocketpy class and performs some operations.
  • If we keep the function now, but delete it in the future, that would be a break change
  • A possible faster solution is to remove its documentation (docstring), thus avoiding breaking changes

I know that @GabrielBarberini would be (or already is) specially interested in using such methods.
So I think we could discuss how to proceed with this before v1 release

@MateusStano
Copy link
Member

What about the add_fin() method of the Rocket class? I think we should delete that one too. It does not work and is not used for anything

@Gui-FernandesBR
Copy link
Member Author

What about the add_fin() method of the Rocket class? I think we should delete that one too. It does not work and is not used for anything

It's not documented tho.
Are you sure it doesn't work? This seem to be such an important feature

@MateusStano
Copy link
Member

It's not documented tho.
Are you sure it doesn't work? This seem to be such an important feature

I have no clue if it works. It might run but I also would have no clue if what it does is even correct

I've never seen it being used, and as far as I know, it was never finished

@Gui-FernandesBR
Copy link
Member Author

It's not documented tho.
Are you sure it doesn't work? This seem to be such an important feature

I have no clue if it works. It might run but I also would have no clue if what it does is even correct

I've never seen it being used, and as far as I know, it was never finished

Yes I agree removing it then. I just read it and there's no sense for that method to be defined inside the Rocket.py file.
We should resolve this in AeroSurface.py

I'm still waiting for a review here, so I can fix all the review comments at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

I understand why these functions are @static_methods and I think this is fine as is, but just for the sake of questioning

Why are these function @static_methods and not functions in utilities.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question!

1- They are used inside other RocketPy classes, so there's no possibility

2- one could suggest to store these methods in tools.py tho. However, I think they are quite related to the Environment class, so it makes more sense if they stay here at this file, together with similar methods. As another example, you can see the reshape_thrust_curve method of the motor class. But this is just a matter of preference tho.

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.

Everything seems good here. I couldn't find anything else that needed deprecation in the code. Just needs to remove add_fin and its ready to go

@Gui-FernandesBR Gui-FernandesBR merged commit 3d7e4a4 into beta/v1.0.0 Sep 23, 2023
8 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the maint/deprecate-methods-to-v1 branch September 23, 2023 13:13
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.

2 participants