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

Panic on semantic error, instead of returning error #31

Merged
merged 1 commit into from
Jul 31, 2019
Merged

Panic on semantic error, instead of returning error #31

merged 1 commit into from
Jul 31, 2019

Conversation

lo48576
Copy link
Contributor

@lo48576 lo48576 commented Jul 31, 2019 •
edited
Loading

Fixes #29 and #30.

@lo48576
Copy link
Contributor Author

lo48576 commented Jul 31, 2019

This is non-breaking change (I think).

@saschagrunert
Copy link
Owner

Thanks for the PR, I'm honestly not sure if a panic is better than returning an error. Usually I would expect that the lib does not panic at all if we return an Fallible<...>.

@lo48576
Copy link
Contributor Author

lo48576 commented Jul 31, 2019 •
edited
Loading

  • If internal data is inconsistent, library user can do nothing about it, because internals are hidden from users and noone (including library developer) knows what happened.
    (If the library developer knows what happened, then it should be fixed before users use it).
  • Additionally, users might have done something using the inconsistent (broken) data, then also any data outside the library might be wrong or inconsistent.
  • Therefore, assertion failure indicates "the program has been already running in unexpected state", and such program is not expected to work correctly even if it does not panic.

Library interface (including error type) represents specification.
Semantic error (bug) is not specification.
Therefore, internal consistency failure should not reported as return value.

@lo48576
Use asserts instead of returning errors on semantic errors
b11bd01 
When the functions noticed inconsistent data, they are broken in
unexpected way and nothing meaningful can be done anymore.
In such cases, the library should panic and all following operations
should be cancelled.

Additionally, now `NodeId::prepend()` works same as `append()` when the
parent node has no children.
(Previously, it is not possible to `prepend()` to the parent node which
has no children yet.)
By this change, their behavior become symmetric and `prepend()` requires
fewer preconditions.

Fixes saschagrunert#29 and saschagrunert#30.
@lo48576 lo48576 force-pushed the feature/panic-on-semantic-error branch from fc7c18d to 58b2ecd Compare July 31, 2019 14:17
saschagrunert
saschagrunert requested changes Jul 31, 2019
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
@lo48576 lo48576 force-pushed the feature/panic-on-semantic-error branch from 58b2ecd to b11bd01 Compare July 31, 2019 16:56
saschagrunert
saschagrunert approved these changes Jul 31, 2019
View reviewed changes
Copy link
Owner

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again

@saschagrunert saschagrunert merged commit cb5e48c into saschagrunert:master Jul 31, 2019
@lo48576 lo48576 deleted the feature/panic-on-semantic-error branch July 31, 2019 17:18
@lo48576 lo48576 mentioned this pull request Aug 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschagrunert saschagrunert saschagrunert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Panic on semantic error, rather than returning error?
2 participants
@lo48576 @saschagrunert