-
Notifications
You must be signed in to change notification settings - Fork 31
feat: @ckb-ccc/molecule #210
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
Conversation
🦋 Changeset detectedLatest commit: 807aef4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution. However, the style of this code is not good enough to be merged into CCC. Issues include:
-
Inconsistent naming -
getCodecMapFromMol
/createCodecMap
's logic are almost always parsing something, but using different verbs.nonNull
as a function, it has no verb in its name, and also different fromassert...
functions likeassertFixedMolType
. Moreover, we havecheck...
functions that return nothing, basicallyassert...
functions. -
Non-descriptive names - Names like
refs
force devs to look into the code to tell what it is. Others liketoMolTypeMap.results
give us a wrong description - it's an argument, not a result. -
Performance - This is the least important issue. But still,
MolType[]
is converted toMolTypeMap
multiple times, and some assertions likeassertFixedMolType
might happen more than once in a single type.
I expect the improved version and will review the demo code after that. In the meantime, the first commit modifies |
Names changes are made. Functions are refactored to keep pure and MolTypeDefinition is not converted to MolTypeMap any more and assertions are also improved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version is way better than the last one! Everything is understandable now.
I commented with some questions and suggestions that might help us improve. Please check.
* feat: compatible mode for molecule decode * Create olive-dryers-live.md --------- Co-authored-by: Hanssen <hanssen0@hanssen0.com>
Commit history seems imreparable. Started a new one at #212 |
Splitted commit for demo