Skip to content
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

feat: add Type ID implementation #108

Merged
merged 12 commits into from
Sep 25, 2024

Conversation

XuJiandong
Copy link
Collaborator

@XuJiandong XuJiandong commented Sep 23, 2024

  • Add implementation of Type ID
  • Add new feature type-id
  • Add test cases
  • Update test dependencies

test/Cargo.toml Outdated Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
Copy link
Collaborator

@mohanson mohanson left a comment

Choose a reason for hiding this comment

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

LGTM

@XuJiandong XuJiandong requested a review from xxuejie September 25, 2024 00:52
Copy link
Collaborator

@xxuejie xxuejie left a comment

Choose a reason for hiding this comment

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

In general this is fine but I'm confused here, it really seems to me that this PR mixes type ID logic with other non-type-id changes together. In the future it would be best if we can split non-related changes to multiple PRs.

@XuJiandong XuJiandong merged commit 1317840 into nervosnetwork:master Sep 25, 2024
1 check passed
@xxuejie
Copy link
Collaborator

xxuejie commented Sep 25, 2024

It really seems to me that a different workflow is used here:

  • Setup a common, separate branch elsewhere amongst a small team
  • Everyone contributes to this separate branch together
  • Periodically, the commits from this separate branch is merged over to upstream here

If this different workflow is used, it should be clearly noted in the PR title / description.

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.

3 participants