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.

Sign up for GitHub

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

Jump to bottom

Converted print statements to errors/warnings; updated tests #137

Merged
merged 4 commits into from
Sep 19, 2023
Merged

Converted print statements to errors/warnings; updated tests #137

merged 4 commits into from
Sep 19, 2023

Conversation

CBROWN-ONS
Copy link
Collaborator

Description

This PR removes the use of print statements across the GTFS module where errors can be used in their place.

Fixes #76

Motivation and Context

The motivation of this PR is to improve consistency across the package and to also ensure that any invalid user inputs are being handled correctly.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor

How Has This Been Tested?

Notes:

  • Pytest passed locally and on the runners.
  • Coverage html report showing no loss of coverage across the gtfs module.
  • Pre-commit passing locally, during commit and also through github actions.

Test configuration details:

  • OS: Windows 10
  • Python version: 3.9.13
  • Java version: N/A
  • Python management system: Conda

Advice for reviewer

I have decided not to remove the print statement in clean_feed() as it can be addressed in #74 . I am also a bit weary about making changes to this function before the additional function calls included in the fast travel validation PR are merged into dev.

Checklist:

  • My code follows the intended structure of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Additional comments

@CBROWN-ONS CBROWN-ONS added x-small technical debt A better way is available. Fix later approach has been adopted. GTFS labels Sep 18, 2023
@CBROWN-ONS CBROWN-ONS added this to the sprint 4 End milestone Sep 18, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.40% 🎉

Comparison is base (87216ed) 87.27% compared to head (f30c6c0) 87.67%.
Report is 7 commits behind head on dev.

Additional details and impacted files 
@@            Coverage Diff             @@
##              dev     #137      +/-   ##
==========================================
+ Coverage   87.27%   87.67%   +0.40%     
==========================================
  Files          11       11              
  Lines         896      917      +21     
==========================================
+ Hits          782      804      +22     
+ Misses        114      113       -1
Flag Coverage Δ
unittests 87.67% <100.00%> (+0.40%) ⬆️

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

Files Changed Coverage Δ
src/transport_performance/gtfs/validation.py 96.62% <100.00%> (+<0.01%) ⬆️

... and 1 file with indirect coverage changes

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

@ethan-moss ethan-moss self-assigned this Sep 19, 2023
ethan-moss
ethan-moss approved these changes Sep 19, 2023
View reviewed changes
Copy link
Collaborator

@ethan-moss ethan-moss left a comment

Choose a reason for hiding this comment

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

All changes LGTM - thanks for raising this PR!

I have checked the individual changes from prints to warnings/errors and they are working as expected. I'm also OK with moving the clean_feed()/stop_id print to error change to #74 - makes a lot of sense to tackle that there. Unit tests updated to adequately cover the changes made in the source code and are performing as expected.

Finally, all unit tests pass with no new warnings (all previously and the changes here). Happy to merge into dev.

@ethan-moss ethan-moss merged commit 87e63f0 into dev Sep 19, 2023
11 checks passed
@ethan-moss ethan-moss deleted the 76-print-to-warning-and-errors branch September 19, 2023 14:08
github-actions bot pushed a commit that referenced this pull request Sep 19, 2023
@ethan-moss
converted print statements to errors/warnings (#137)
bc497ea 
* Converted print statements to errors/warnings; updated tests

* fix: Convert error to warning

* test: reinstate not impletmented extension test; added check for warning

* test: removed duplicate .svg test

---------

Co-authored-by: Ethan Moss <[email protected]> 87e63f0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethan-moss ethan-moss ethan-moss approved these changes

Assignees

@ethan-moss ethan-moss

Labels
GTFS technical debt A better way is available. Fix later approach has been adopted. x-small
Projects
None yet
Milestone
sprint 4 End
Development

Successfully merging this pull request may close these issues.

change gtfs/validation.py print statements to be errors
3 participants
@CBROWN-ONS @codecov-commenter @ethan-moss