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

chore: identify::Config fields private #5663

Merged
merged 6 commits into from
Nov 6, 2024
Merged

Conversation

kamuik16
Copy link
Contributor

@kamuik16 kamuik16 commented Nov 5, 2024

Description

Closes #5660

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started! We should also define the setter methods, increment the version on Cargo.toml and introduce the change on CHANGELOG.md

@kamuik16
Copy link
Contributor Author

kamuik16 commented Nov 5, 2024

Thanks for getting this started! We should also define the setter methods, increment the version on Cargo.toml and introduce the change on CHANGELOG.md

For setters do you think all the with_* methods that already exists are enough?

@jxs
Copy link
Member

jxs commented Nov 5, 2024

For setters do you think all the with_* methods that already exists are enough?

ah! Good point, I wasn't aware they already existed, yeah makes sense

@kamuik16
Copy link
Contributor Author

kamuik16 commented Nov 6, 2024

Thanks for getting this started! We should also define the setter methods, increment the version on Cargo.toml and introduce the change on CHANGELOG.md

Incremented the version in Cargo.toml and updated CHANGELOG.md accordingly.

@kamuik16
Copy link
Contributor Author

kamuik16 commented Nov 6, 2024

semver check is failing because it require a major version update rather than a minor one. any thoughts?

@kamuik16 kamuik16 requested a review from jxs November 6, 2024 06:41
@stormshield-frb
Copy link
Contributor

Hi @kamuik16. You can bump to 0.46.0. We discussed it with @jxs yesterday. This PR relates to this comment #5574 (comment).

Copy link
Contributor

@stormshield-frb stormshield-frb left a comment

Choose a reason for hiding this comment

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

Left some minor comments about the return value of some getters. It is good practices to return only a reference (in a getter function) if the type is not Copy. It avoids cloning when not required.

protocols/identify/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/identify/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/identify/src/behaviour.rs Outdated Show resolved Hide resolved
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

@jxs jxs added the send-it label Nov 6, 2024
@mergify mergify bot merged commit 858a4cd into libp2p:master Nov 6, 2024
69 checks passed
@kamuik16 kamuik16 deleted the 5660 branch November 6, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make identify::Config fields private
3 participants