std.csv assert messages #7038
std.csv assert messages #7038
Conversation
|
Thanks for your pull request, @burner! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + phobos#7038" |
thewilsonator
added
the
72h no objection -> merge
|
@thewilsonator thank you |
std/csv.d
Outdated
| @@ -989,7 +989,7 @@ public: | |||
| */ | |||
| @property auto front() | |||
| { | |||
| assert(!empty); | |||
| assert(!empty, "An empty range has no front element"); | |||
There was a problem hiding this comment.
More consistent with the messages in range code (std/range/package.d) would be:
"Attempting to fetch the front of an empty CsvReader"
std/csv.d
Outdated
| @@ -1224,7 +1224,7 @@ public: | |||
| */ | |||
| @property Contents front() @safe pure | |||
| { | |||
| assert(!empty); | |||
| assert(!empty, "An empty range has no front element"); | |||
There was a problem hiding this comment.
More consistent with the messages in range code (std/range/package.d) would be:
"Attempting to fetch the front of an empty CsvRecord"
std/csv.d
Outdated
| @@ -1425,7 +1425,7 @@ if (isSomeChar!Separator && isInputRange!Range | |||
|
|
|||
| while (!input.empty) | |||
| { | |||
| assert(!(quoted && escQuote)); | |||
| assert(!(quoted && escQuote), "Invalid quotation state"); | |||
There was a problem hiding this comment.
Consider adding the function to the message in some fashion. This is often helpful when debugging. eg.
"Invalid quotation state in csvNextToken"
There was a problem hiding this comment.
Just realized this is part of a much larger effort to add messages to all asserts. (Fantastic!) So, ignore my comment about this specific case and instead be consistent across all cases.
FWIW, I've benefitted when the assert messages include a reference to the code construct that is asserting. This has even been helpful to tracking down bugs in Phobos. But most important is that there is an assert message.
burner
force-pushed
the
std.csv_assert_messages
branch
from
24b053d
to
001bfe7
Compare
|
@jondegenhardt thank you for the review |
|
LGTM. Thanks for making the changes. |
|
@jondegenhardt that's what review is for |
thewilsonator
added
auto-merge
and removed
72h no objection -> merge
|
@burner please do remind me about these when I forget, (unless you've forgotten too...). |
|
@thewilsonator sure thing, thanks |
No description provided.