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

BUG: Function breaks if a header is present in the csv file #485

Merged
merged 10 commits into from
Nov 27, 2023

Conversation

phmbressan
Copy link
Collaborator

@phmbressan phmbressan commented Nov 25, 2023

Pull request type

  • Code changes (bugfix, features)
  • ReadMe, Docs and GitHub updates

Checklist

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

Current behavior

The Function.__check_user_inputs did not support headered csv, which caused an error on docs build.

New behavior

The support for headered csv was added and there was a LiquidMotor example in the docs that needed refactor to the new cap height definition.

Breaking change

  • No

@phmbressan phmbressan added Bug Something isn't working Docs Docs and examples related labels Nov 25, 2023
@phmbressan phmbressan self-assigned this Nov 25, 2023
Copy link

codecov bot commented Nov 25, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (6015dcc) 70.89% compared to head (cd9d10c) 70.91%.
Report is 1 commits behind head on master.

Files Patch % Lines
rocketpy/mathutils/function.py 69.23% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #485      +/-   ##
==========================================
+ Coverage   70.89%   70.91%   +0.02%     
==========================================
  Files          55       55              
  Lines        9258     9261       +3     
==========================================
+ Hits         6563     6567       +4     
+ Misses       2695     2694       -1     
Flag Coverage Δ
unittests 70.91% <69.23%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

- single line headers are now officially supported...
- with or without quotes.
- The docstring was updated.
@Gui-FernandesBR Gui-FernandesBR changed the title BUG: Hotfix v1.1.1 Docs Build - Function CSV BUG: Function breaks if a header is present in the csv file Nov 25, 2023
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.

LGTM!

All tests are passing locally, including with the --run-slow option

The check_user_inputs method is raising more problems than what I thought in the beginning. Improvements for this method are welcome, and I believe there's already an issue for that.

Good news: docs are passing! See https://docs.rocketpy.org/en/bug-v1.1.1-docs-build/

rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
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.

This Looks Good To Me

rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
@phmbressan phmbressan merged commit 0ef9849 into master Nov 27, 2023
12 of 13 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the bug/v1.1.1-docs-build branch December 3, 2023 05:08
@phmbressan phmbressan mentioned this pull request Dec 19, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Docs Docs and examples related
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

4 participants