-
Notifications
You must be signed in to change notification settings - Fork 10
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
Rewrite library #12
Open
sterliakov
wants to merge
62
commits into
vechain:master
Choose a base branch
from
sterliakov:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Rewrite library #12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Thanks for the submission! Nice work. I will take a look at it page by page. |
By the way, can you squash the commits together so I can start make comments on it? |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is part of VeChain hackathon submission.
Core changes
mypy
-compatible code, includingTypedDict
for data static validation;camelCase
methods exposed,validate
andis_valid
with consistent meaning);docs/source/ext
files docstrings for explanation);Event
andFunction
interface:dot.access
by name, numericindex[access]
andto_dict
method to convert to{name: value}
dictionary);README
);ScalarKind
andBaseWrapper
interface: they both provideserialize
anddeserialize
public methods;Certificate
verify
andencode
are now methods instead of module-level functions, because they were taking a single argument,Certificate
instance, and thus are perfect methods.HDNode
subclasses can now override constants;solidity
happens only once per version);pytest
code with fixtures.Current docs
Hosted on RTD
I suggest that the library maintainer will create ReadTheDocs account and deploy docs. All configuration is already done (see
.readthedocs.yaml
). Then link inREADME
should be updated.Why so much about style?
Because library code will be read a lot, so consistent code formatting eases the process. Why
black
? Because it formats everything with the same rules, so any contributor will not add his own style to the project codebase. Andflake8
ensures that no bad naming policy and no invalid/missing documentation will be present in repository.Why
mypy
was chosen as a reference type checker?Because it is de-facto standard in python ecosystem, the tool is maintained by python core team. It is most likely included in our users pipeline.
Why properties?
Because it is python. While not enforced by
pep8
, properties are great way in python to express some calculated attributes of object, hiding the internals from end user.selector
orid
of something is not a function - you don't try to call it, you try to access it. Thus, properties are semantically more suitable.Why supporting ancient
bip-utils
?Because we support python 3.6 and it is the latest version available for 3.6. Remember, that python 3.6 is End Of Life more than a year, right? And
bip_utils 2.0.0
introduced major backwards incompatible changes. It would be great to drop 3.6 support and upgrade dependencies unconditionally, and I'm ready to work on that given maintainer's confirmation.Coverage with tox
Impact
Closes #10
Closes #11