Skip to content
This repository was archived by the owner on Sep 10, 2019. It is now read-only.

Conversation

@jseagrave21
Copy link
Contributor

@jseagrave21 jseagrave21 commented Mar 5, 2019

What current issue(s) does this address, or what feature is it adding?
See CityOfZion/neo-python#888

How did you solve this problem?

How did you make sure your solution works?
make test

Did you add any tests?
only updated tests

edit
Added 1 test to increase Crypto.py coverage to 100%

Did you run make lint and make coverage?
Yes

Are there any special changes in the code that we should be aware of?
No

- per #888:
  - removed logzero dependency
  - updated and clarified exception types raised
    - implemented OrdinalError, MessageError, and LegendaireExponentError
Copy link
Member

@ixje ixje left a comment

Choose a reason for hiding this comment

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

Overall good changes. There's a couple of inline comments I made and there's one more request I'd like to do here: Please update the docstrings to include the exceptions raised.

I know they're not always documented, and some might now have changed from Exception to ValueError or whatever. But we should really add them. See the style here: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

something along the lines of

Raises: 
   <exception type>: <description/cause>

Copy link
Member

@ixje ixje left a comment

Choose a reason for hiding this comment

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

Almost there. Just a couple of docstring suggestions/requests. The rest looks good :)

@ixje
Copy link
Member

ixje commented Mar 6, 2019

Before I can merge this we must update neo-python to support these changes. That way we can orchestrate a simultaneous new release of core + main

@jseagrave21
Copy link
Contributor Author

Okay. I will start working on neo-python as well. Perhaps, I will be able to point make test to pull from my new branch like we did in CityOfZion/neo-python#716

@coveralls
Copy link

coveralls commented Mar 7, 2019

Coverage Status

Coverage increased (+0.08%) to 96.614% when pulling 45974b0 on jseagrave21:update-exceptions into 786c02c on CityOfZion:master.

@jseagrave21
Copy link
Contributor Author

I fixed the issues created by the updates in this branch CityOfZion/neo-python@master...jseagrave21:support-#888.

- increase Crypto.py coverage to 100%
@jseagrave21
Copy link
Contributor Author

After adding the above test, I am wondering if every user will understand that the message here

message (str): message to be signed
should be a hexstr while the message here
message (str): the message to verify.
could be a hexstr or a normal str.

@ixje
Copy link
Member

ixje commented Mar 7, 2019

From the 2 lines you've copied I cannot tell this, so that's already enough to indicate we need to clarify it.

To give you an idea what the docstrings look like in an IDE

Left upper balloon is the quick hints you get. If you don't understand the purpose from that you can request the extensive documentation that get formatted like the bottom right balloon.

@jseagrave21
Copy link
Contributor Author

@ixje okay, I think it should be good

@jseagrave21
Copy link
Contributor Author

Based on achieving a passing build and resolving all reviews/comments, I am reverting travis.yml to pull from the development branch. Please see CityOfZion/neo-python#911 for the neo-python PR.

Copy link
Member

@ixje ixje left a comment

Choose a reason for hiding this comment

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

see reasoning in comments

@ixje ixje changed the base branch from master to development March 18, 2019 08:24
@ixje ixje merged commit d61ed6e into CityOfZion:development Mar 18, 2019
@ixje
Copy link
Member

ixje commented Mar 18, 2019

200 points jseagrave

@jseagrave21 jseagrave21 deleted the update-exceptions branch March 18, 2019 09:00
@lllwvlvwlll
Copy link
Member

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants