Skip to content

Conversation

Alive24
Copy link
Contributor

@Alive24 Alive24 commented May 17, 2025

Splitted commit for demo

Copy link

changeset-bot bot commented May 17, 2025

🦋 Changeset detected

Latest commit: 807aef4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 21 packages
Name Type
@ckb-ccc/core Minor
@ckb-ccc/eip6963 Patch
@ckb-ccc/joy-id Patch
@ckb-ccc/lumos-patches Patch
@ckb-ccc/molecule Patch
@ckb-ccc/nip07 Patch
@ckb-ccc/okx Patch
@ckb-ccc/rei Patch
@ckb-ccc/shell Patch
@ckb-ccc/spore Patch
@ckb-ccc/ssri Patch
@ckb-ccc/udt Patch
@ckb-ccc/uni-sat Patch
@ckb-ccc/utxo-global Patch
@ckb-ccc/xverse Patch
@ckb-ccc/ccc Patch
ckb-ccc Patch
@ckb-ccc/connector Patch
@ckb-ccc/examples Patch
@ckb-ccc/ccc-playground Patch
@ckb-ccc/connector-react Patch

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

Copy link
Member

@Hanssen0 Hanssen0 left a 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:

  1. 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 from assert... functions like assertFixedMolType. Moreover, we have check... functions that return nothing, basically assert... functions.

  2. Non-descriptive names - Names like refs force devs to look into the code to tell what it is. Others like toMolTypeMap.results give us a wrong description - it's an argument, not a result.

  3. Performance - This is the least important issue. But still, MolType[] is converted to MolTypeMap multiple times, and some assertions like assertFixedMolType might happen more than once in a single type.

@Hanssen0
Copy link
Member

I expect the improved version and will review the demo code after that. In the meantime, the first commit modifies pnpm-lock, but CI seems to have some problems. Please check.

Alive24 added a commit to Alive24/ccc that referenced this pull request May 17, 2025
@Alive24 Alive24 requested a review from Hanssen0 May 17, 2025 22:37
Alive24 added a commit to Alive24/ccc that referenced this pull request May 17, 2025
Alive24 added a commit to Alive24/ccc that referenced this pull request May 17, 2025
Alive24 added a commit to Alive24/ccc that referenced this pull request May 18, 2025
Alive24 added a commit to Alive24/ccc that referenced this pull request May 18, 2025
@Alive24
Copy link
Contributor Author

Alive24 commented May 19, 2025

Thank you for the contribution. However, the style of this code is not good enough to be merged into CCC. Issues include:

  1. 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 from assert... functions like assertFixedMolType. Moreover, we have check... functions that return nothing, basically assert... functions.
  2. Non-descriptive names - Names like refs force devs to look into the code to tell what it is. Others like toMolTypeMap.results give us a wrong description - it's an argument, not a result.
  3. Performance - This is the least important issue. But still, MolType[] is converted to MolTypeMap multiple times, and some assertions like assertFixedMolType might happen more than once in a single type.

Names changes are made. Functions are refactored to keep pure and MolTypeDefinition is not converted to MolTypeMap any more and assertions are also improved.

Copy link
Member

@Hanssen0 Hanssen0 left a 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.

@Alive24 Alive24 requested a review from Hanssen0 May 20, 2025 22:21
Alive24 added a commit to Alive24/ccc that referenced this pull request May 24, 2025
Alive24 added a commit to Alive24/ccc that referenced this pull request May 24, 2025
Alive24 added a commit to Alive24/ccc that referenced this pull request May 24, 2025
Alive24 added a commit to Alive24/ccc that referenced this pull request May 24, 2025
@Alive24
Copy link
Contributor Author

Alive24 commented May 24, 2025

Commit history seems imreparable. Started a new one at #212

@Alive24 Alive24 closed this May 24, 2025
@Alive24 Alive24 deleted the feat-molecule branch May 24, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants