Skip to content

MNT: update Makefile #539

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

Merged
merged 5 commits into from
Feb 8, 2024
Merged

MNT: update Makefile #539

merged 5 commits into from
Feb 8, 2024

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

  • ReadMe, Docs and GitHub updates

Checklist

  • Tests for the changes have been added (if needed) [not relevant]
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant) [not relevant]

Current behavior

The current Makefile is dated from more than 2 years ago, and I'm under the impression that almost none of our developers (including me) use it so often.

New behavior

I updated (and hopefully improved) the Makefile, now adding new commands to help us during development phase.
You can now use the following commands:

  • make pytest
  • make slow
  • make coverage
  • make coverage-report
  • make install
  • make isort
  • make black
  • make pylint
  • make build-docs

Breaking change

  • No

Additional information

  • There's a strange error happening when running make pylint, but it is being ignored by the - sign and should not affect the performance, so don't worry about that.
  • Please ensure the commands are running properly in your local machines! I'm curious to see the behaviour when running in Windows, @MateusStano .

@Gui-FernandesBR Gui-FernandesBR changed the base branch from master to develop January 25, 2024 16:55
@Gui-FernandesBR Gui-FernandesBR requested a review from a team as a code owner January 25, 2024 16:55
@Gui-FernandesBR Gui-FernandesBR added the Git housekeeping Clean and organize our github label Jan 25, 2024
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Jan 25, 2024
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (435bd62) 72.32% compared to head (5234f6d) 72.32%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #539   +/-   ##
========================================
  Coverage    72.32%   72.32%           
========================================
  Files           56       56           
  Lines         9399     9399           
========================================
  Hits          6798     6798           
  Misses        2601     2601           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phmbressan
Copy link
Collaborator

I commited a mistake in which I removed the - sign you mentioned in the PR description. After noticing the problem, I returned things as they were.

However, after commiting, this triggered the lint GitHub action which oddly disagreed from the black of my PC. So it changed the formatting of some non-related files.

Do you have any ideas why this might be happening @Gui-FernandesBR ?

@Gui-FernandesBR
Copy link
Member Author

I commited a mistake in which I removed the - sign you mentioned in the PR description. After noticing the problem, I returned things as they were.

However, after commiting, this triggered the lint GitHub action which oddly disagreed from the black of my PC. So it changed the formatting of some non-related files.

Do you have any ideas why this might be happening @Gui-FernandesBR ?

that's weird man. Let's update the branch with a rebase and wait to see what is happening

@Gui-FernandesBR
Copy link
Member Author

Ok, I understand it now, there's a new version of black out there, this is creating some formatting differences (https://pypi.org/project/black/).

I think it is OK to include the linter changes in this PR, no big deal.

Please update your black: pip install black --upgrade

@phmbressan
Copy link
Collaborator

@MateusStano could you test some of these commands in Windows, because I am afraid changing the Python command from python to python3 might cause some issues.

@MateusStano
Copy link
Member

@MateusStano could you test some of these commands in Windows, because I am afraid changing the Python command from python to python3 might cause some issues.

Yep, python3 does not work on windows

@MateusStano
Copy link
Member

By the way, it seems that the requirements-tests.txt file is requiring pytest==6.2.4. Pytest is already in version 8.0, might be good to change that

@phmbressan
Copy link
Collaborator

By the way, it seems that the requirements-tests.txt file is requiring pytest==6.2.4. Pytest is already in version 8.0, might be good to change that

I have removed the versioning cap and the tests still ran fine. If there was a specific reason to keep the cap, please correct me.

@phmbressan
Copy link
Collaborator

@MateusStano could you test some of these commands in Windows, because I am afraid changing the Python command from python to python3 might cause some issues.

Yep, python3 does not work on windows

I believe I have found a solution to set the Environment variable according the common definition of the UNIX and Windows. Of course, if the user changed that it will fail.

Could you test them on Windows again, just to make sure it is working?

@Gui-FernandesBR
Copy link
Member Author

Great adjustments, I wanted to confirm the code is working on linux.

Asking help from windows developers here @MateusStano @giovaniceotto @lucasfourier @juliomachad0

It is latterly checking out the mnt/update-makefile branch and running "make X" as described in the PR description.
I'm gonna restore my windows machine in the short-term future to avoid inconveniences like this.

@Gui-FernandesBR
Copy link
Member Author

Just tested on windows now with @juliomachad0 's help, everything works, amazing!

@Gui-FernandesBR Gui-FernandesBR merged commit bd6362c into develop Feb 8, 2024
@Gui-FernandesBR Gui-FernandesBR deleted the mnt/update-makefile branch February 8, 2024 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Git housekeeping Clean and organize our github
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants