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

Replace error chain #25

Merged
merged 10 commits into from
Nov 16, 2018
Merged

Replace error chain #25

merged 10 commits into from
Nov 16, 2018

Conversation

zrzka
Copy link
Contributor

@zrzka zrzka commented Nov 15, 2018

Fixes #24

PR contains:

  • removed explicit notifications from .travis.yml
  • added @cyplo as an author
  • replaced error-chain with custom error type [1]
  • integrations tests imports organized

[1] This is ongoing work, because I'd like to add more info to error messages (like the whole expression, etc.), but it's kinda okay for now. It's nothing that should block us - internal stuff, no public API breakage.

Why custom error and not failure for example? error-chain was deprecated and failure recommended. The problem here is that even the failure is not magic & final solution, there's still some ongoing work, ... Also I think that we should use custom error types for libs, which do comply with std::error::Error. The reason I was using error-chain was simple - speed & experimentation. Now, when things are settling down, there's no reason to use it.

Robert Vojta added 6 commits November 15, 2018 15:18
Change-type: patch
Signed-off-by: Robert Vojta <robert@balena.io>
Signed-off-by: Robert Vojta <robert@balena.io>
Signed-off-by: Robert Vojta <robert@balena.io>
Signed-off-by: Robert Vojta <robert@balena.io>
Signed-off-by: Robert Vojta <robert@balena.io>
Signed-off-by: Robert Vojta <robert@balena.io>
@zrzka zrzka requested a review from cyplo November 15, 2018 14:39
Copy link
Contributor

@cyplo cyplo left a comment

Choose a reason for hiding this comment

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

Pretty cool stuff :)

  • Thank you for the author mention, not sure if I deserve that (yet;)
  • have some notes but nothing major :)

src/builtin/filter/datetime.rs Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
Robert Vojta added 3 commits November 16, 2018 08:44
Signed-off-by: Robert Vojta <robert@balena.io>
Signed-off-by: Robert Vojta <robert@balena.io>
Signed-off-by: Robert Vojta <robert@balena.io>
@cyplo
Copy link
Contributor

cyplo commented Nov 16, 2018

lgtm 👍

Signed-off-by: Robert Vojta <robert@balena.io>
@zrzka zrzka merged commit 1933c39 into master Nov 16, 2018
@zrzka zrzka deleted the replace-error-chain branch November 16, 2018 09:46
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.

None yet

2 participants