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

Put the subcmd name in the validate_output error message [#1539] #1540

Merged
merged 2 commits into from
Jul 12, 2024

Conversation

genehack
Copy link
Contributor

@genehack genehack commented Jul 12, 2024

Description of proposed changes

This puts the full sub-command name into the error message shown when the output records have inconsistent field names.

Related issue(s)

#1539

Checklist

  • Automated checks pass
  • Check if you need to add a changelog message
  • Check if you need to add tests
  • Check if you need to update docs

@genehack genehack requested a review from a team July 12, 2024 00:21
Copy link

codecov bot commented Jul 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.90%. Comparing base (81badd2) to head (58db430).
Report is 176 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1540      +/-   ##
==========================================
- Coverage   69.90%   69.90%   -0.01%     
==========================================
  Files          75       75              
  Lines        7882     7888       +6     
  Branches     1933     1933              
==========================================
+ Hits         5510     5514       +4     
- Misses       2086     2088       +2     
  Partials      286      286              

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

Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

My comment in #1518 came back to bite real quick 😅

there's no good way to check for the output error. Hopefully we just never see it!

augur/curate/__init__.py Outdated Show resolved Hide resolved
@genehack
Copy link
Contributor Author

My comment in #1518 came back to bite real quick 😅

there's no good way to check for the output error. Hopefully we just never see it!

I spent like, 30 seconds thinking about mocking something up to cause the error so I could write a test, then settled for manually validating it on the broken version of parse-genback-location...

@joverlee521
Copy link
Contributor

Once this is merged, I think good to release along with #1531 as Augur 25.1.1

(Probably wait till Monday so we don't do a Friday release 🙈)

@genehack
Copy link
Contributor Author

I spent like, 30 seconds thinking about mocking something up to cause the error so I could write a test, then settled for manually validating it on the broken version of parse-genback-location...

In the harsh light of morning, I realized, while functional tests would be a PITA, I could totally trivially unit test the validate_records() method, so I added that.

@genehack genehack merged commit df7859e into master Jul 12, 2024
23 checks passed
@genehack genehack deleted the better-error-message-1539 branch July 12, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants