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.
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.nonNullas 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
refsforce devs to look into the code to tell what it is. Others liketoMolTypeMap.resultsgive us a wrong description - it's an argument, not a result. -
Performance - This is the least important issue. But still,
MolType[]is converted toMolTypeMapmultiple times, and some assertions likeassertFixedMolTypemight 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. |
Hanssen0
left a comment
There was a problem hiding this comment.
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