multiformats / py-multiaddr

multiaddr implementation in Python
https://multiformats.io/
Other
33 stars 23 forks source link

Rethink the exception hierarchy #41

Closed ntninja closed 5 years ago

ntninja commented 5 years ago

Now looks like this:

builtins.Exception
 +-- builtins.ArithmeticError
 |    +-- builtins.OverflowError (Integer was too large during parsing)
 +-- builtins.TypeError (Invalid parameter type)
 +-- builtins.LookupError
 |    +-- LookupError
           +-- ProtocolLookupError (MultiAddr did not contain the requested protocol)
 +-- builtins.ValueError
 |    +-- ParseError
 |         +-- BinaryParseError (Failed to parse the binary representation)
 |         +-- StringParseError (Failed to parse the text/string representation)
 +-- ProtocolManagerError
      +-- ProtocolExistsError (Tried to add protocol already defined)
      +-- ProtocolNotFoundError (Tried to find non-existing protocol)

Additionally all exceptions defined by py-multiaddr also derive from multiaddr.exceptions.Error.

Not everything ended up being ValueError, but all the usual try: MultiAddr(string); except ValueError: … style cases did.

Fixes #39

codecov-io commented 5 years ago

Codecov Report

Merging #41 into master will increase coverage by 1.05%. The diff coverage is 100%.

@@           Coverage Diff            @@
##           master    #41      +/-   ##
========================================
+ Coverage   98.94%   100%   +1.05%     
========================================
  Files          12     13       +1     
  Lines         380    409      +29     
  Branches       55     54       -1     
========================================
+ Hits          376    409      +33     
+ Misses          2      0       -2     
+ Partials        2      0       -2
ntninja commented 5 years ago

Done! We now have 100% coverage!

ntninja commented 5 years ago

I think we should have the consensus of the indentation. Personally, I am used to spaces. But it's just fine that we all agree on the same indentation, no matter which one is used. What is your thought?

I'm biased towards tabs, makes it easier to get a properly sized (in my case ~4 spaces wide) indentation no matter what the project devs or their community's favourite coding standard prefers. Also useful because in some contexts (3-way merges mostly) you can then easily make lines a lot shorter, at the cost of some readability anyways, when you need the extra horizontal space. Just my 2¢s anyways.

Tell me which style I should apply and I'll do the necessary adjustments.

mhchia commented 5 years ago

I'm biased towards tabs, makes it easier to get a properly sized (in my case ~4 spaces wide) indentation no matter what the project devs or their community's favourite coding standard prefers. Also useful because in some contexts (3-way merges mostly) you can then easily make lines a lot shorter, at the cost of some readability anyways, when you need the extra horizontal space. Just my 2¢s anyways.

Sounds reasonable. I didn't realize these pros before. However, since a large portion of the code is indented with spaces, would you please apply spaces(change all tabs to spaces) for now? Maybe it worth an issue to discuss it in the future. Thank you.

ntninja commented 5 years ago

However, since a large portion of the code is indented with spaces, would you please apply spaces(change all tabs to spaces) for now?

Done! It was 611 to 214 changes for spaces so I applied that.