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 NotImplementedErrors with MethodErrors #27

Closed
nickrobinson251 opened this issue May 27, 2020 · 1 comment · Fixed by #28
Closed

Replace NotImplementedErrors with MethodErrors #27

nickrobinson251 opened this issue May 27, 2020 · 1 comment · Fixed by #28

Comments

@nickrobinson251
Copy link
Contributor

nickrobinson251 commented May 27, 2020

  • MethodErrors convey this same information, with the bonus of showing "closest candidates"
  • Since all the functions here belong to the DBInterface namespace (we're not e.g. adding methods to Base functions), the MethodErrors should be particularly helpful
  • Furthermore, in a few cases the message in the NotImplementedError may just be incorrect
    • e.g. in
       connect(T, args...; kw...) = throw(NotImplementedError("`DBInterface.connect` not implemented for `$T`"))
      there may be methods for T, but not for the given args
    • this is discussed at the start of this blog https://white.ucc.asn.au/2020/04/19/Julia-Antipatterns.html
    • instead we can just define a function with a docstring and no methods
      """docstring"""
      function connect end
  • This also would remove quite a few lines of code at no cost
@quinnj
Copy link
Member

quinnj commented Jun 5, 2020

Yeah, I'd be fine w/ doing this if you want to put up a PR

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 a pull request may close this issue.

2 participants