-
-
Notifications
You must be signed in to change notification settings - Fork 725
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
adds assert messages for container.array #7190
adds assert messages for container.array #7190
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#7190" |
Thanks for doing this. These assert messages are really valuable, they have helped me many times. The text of the assert messages could be improved by changing the text to be more consistent with the other Array asserts. This would also make them consistent with the most common assert message format used in Phobos range methods. Most valuable would be to include the data type in the assert message. In this case, "Array". Specific suggestions for the first few cases:
The style is taken from examples found by grep'ing Phobos source files for There are many range assert messages that don't adhere to this style, but it appears to be the most common. A somewhat common difference occurs with
|
To clarify my previous comment - I'm fine with the PR being merged as is if the author doesn't want to invest additional time in further improvements to the text. This PR is a meaningful improvement over the current assert messages (none). I looked through them all and didn't see any where the text was inconsistent with the method. A separate thing - I do wonder if the memory allocation failure cases should be asserts. Not sure what the standard mechanism is in Phobos. However, they are asserts currently, this PR is not attempting to look at that, just the assert message text. |
@jondegenhardt thank you for the review. I'll keep a look later this week. |
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.
Very nice. Thanks for making these updates.
@jondegenhardt thanks for the review |
@n8sh @atilaneves @jondegenhardt thank you |
(Merging manually as buildkite failure is not related to this PR.) |
@PetarKirov thank you |
No description provided.