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

refactor(blockservice, merkledag, namesys) deprecate u.ErrNotFound #224

Merged
merged 6 commits into from
Oct 28, 2014

Conversation

btc
Copy link
Contributor

@btc btc commented Oct 28, 2014

In a couple places we have the choice between returning an exported error and returning an error with information. Which do we prefer? @jbenet @whyrusleeping

@@ -103,7 +103,7 @@ func (n *Node) RemoveNodeLink(name string) error {
return nil
}
}
return u.ErrNotFound
return fmt.Errorf("merkledag: %s not found", name)
Copy link
Member

Choose a reason for hiding this comment

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

make it a static error so we can check against it.

Copy link
Member

Choose a reason for hiding this comment

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

after your last commits this is not a static error yet. it should be so we can compare

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mistake. one moment

@jbenet
Copy link
Member

jbenet commented Oct 28, 2014

@maybebtc its ok if the error doesnt show the name. being able to compare is the important part. (go solved this incorrectly IMO. errors with params should be comparable statically. this doesnt currently leverage the type system well.


experimenting....

type ErrNotFound struct {
  key string
}

func (e *ErrNotFound) Error() string {
  return fmt.Sprintf("merkledag: %s not found", e.key)
}

// elsewhere
foo, err := doSomething()
if err != nil {
  if _, ok err.(mdag.ErrNotFound); ok {
    // this is a not found error.
  }
}

@whyrusleeping @maybebtc thoughts on this pattern \o ? it's annoyingly verbose, but it gives us ability to make errors with params that we can still compare. idk i havent thought about it more than 2 min. this may not be needed at all.

@btc
Copy link
Contributor Author

btc commented Oct 28, 2014

@jbenet If, in the future, we feel pain and need the information in the error, this is a viable solution. The verbosity may only be worth it if we're feeling real pain.

For now, we have enough information to get errors back out the the user. #alphafocus

aside, can also do:

switch err := err.(type) {
case ParseError:
  PrintParseError(err)
}

jbenet added a commit that referenced this pull request Oct 28, 2014
refactor(blockservice, merkledag, namesys) deprecate u.ErrNotFound
@jbenet jbenet merged commit 3a0bbe2 into master Oct 28, 2014
@jbenet jbenet removed the codereview label Oct 28, 2014
@jbenet jbenet deleted the issue-209-plus branch October 28, 2014 23:11
@jbenet
Copy link
Member

jbenet commented Oct 28, 2014

@maybebtc i usually use https://github.com/jbenet/go-ipfs/pull/224/files to CR and see exactly what's changed.

@whyrusleeping
Copy link
Member

Ive been toying with the idea of a wrapped error interface:

type WrappedError interface {
    Error() string
    Root() error
}

func WrapError(err error, context interface{}) error {
    ...
}

This way we could have more information about where each error origination, and still be able to compared the root error types.

@jbenet
Copy link
Member

jbenet commented Oct 28, 2014

I like this. there really should be a more sophisticated Error interface, like context.Context. shrug not a big deal atm.

@whyrusleeping
Copy link
Member

Ill keep it around, maybe after the alpha I can work on refitting our error handling system

@aschmahmann aschmahmann mentioned this pull request Sep 22, 2020
72 tasks
ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Oct 23, 2021
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
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.

3 participants