-
Notifications
You must be signed in to change notification settings - Fork 129
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
merge: polish #1591
merge: polish #1591
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1591 +/- ##
==========================================
+ Coverage 70.51% 70.56% +0.04%
==========================================
Files 78 78
Lines 8107 8119 +12
Branches 1966 1967 +1
==========================================
+ Hits 5717 5729 +12
Misses 2100 2100
Partials 290 290 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
This is a small thing, really, but it's a detail that helps make error messages more precise and thus potentially less confusing. It's easy enough, so take the care to do it.
Format defaults so they're formatted for the shell like would be seen/input on the command-line by humans instead of as a Python value. Rephrases default for argument-less --quiet flag. Suppresses "(default: None)" for required options.
e13659b
to
de1bf9a
Compare
Rebased onto latest |
One more bit of error polishing coming in a few minutes… |
Demonstrates the current (not ideal) error message.
This message in Metadata() now matches the error message for the same condition in read_metadata(), which is a big improvement to the "say what now??" quotient. It seems like Metadata's _find_first() implementation was intentionally made generic thus requiring a generic error message, but it's only used in a single place, to find the id column, so make it specific and thus clearer.
… error messages The tuple(…)!r trick is cute but leaves single-item lists with a trailing comma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Added a few non-blocking comments which would be good to address post-merge, maybe filed as new issues. I can write those up / just do it if it's easy enough.
See commit messages.
Checklist