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.

Sign up for GitHub

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

Jump to bottom

chore: export p2p package errors #1901

Merged
merged 32 commits into from
Mar 18, 2024
Merged

chore: export p2p package errors #1901

merged 32 commits into from
Mar 18, 2024

Conversation

akaladarshi
Copy link
Contributor

This PR Contributes to: #1140

Changes:

  • Exports errors for p2p package

@akaladarshi akaladarshi requested review from a team as code owners December 28, 2023 13:52
melekes
melekes reviewed Jan 4, 2024
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

Thanks @Tikaryan ❤️

p2p/conn/connection.go Outdated Show resolved Hide resolved
p2p/conn/errors.go Outdated Show resolved Hide resolved
p2p/conn/errors.go Outdated Show resolved Hide resolved
p2p/conn/errors.go Outdated Show resolved Hide resolved
p2p/conn/errors.go Outdated Show resolved Hide resolved
p2p/errors.go Outdated Show resolved Hide resolved
p2p/errors.go Outdated Show resolved Hide resolved
p2p/errors.go Outdated Show resolved Hide resolved
p2p/errors.go Outdated Show resolved Hide resolved
p2p/errors.go Outdated Show resolved Hide resolved
@cason cason mentioned this pull request Jan 8, 2024
Open
20 tasks
@adizere adizere added this to the 2024-Q1 milestone Jan 19, 2024
@cason
Copy link
Contributor

cason commented Jan 22, 2024

As a general comment here, one of the points of exporting errors (types instead of strings) is to improve testing. Some test units (not most, unfortunately), attest the production of an error in certain scenarios. The introduced errors should thus be used in the existing test units.

cason
cason reviewed Jan 22, 2024
View reviewed changes
p2p/conn/secret_connection.go Outdated Show resolved Hide resolved
p2p/conn/secret_connection.go Outdated Show resolved Hide resolved
p2p/conn/errors.go Outdated Show resolved Hide resolved
p2p/errors.go Outdated Show resolved Hide resolved
p2p/pex/pex_reactor.go Outdated Show resolved Hide resolved
p2p/switch.go Outdated Show resolved Hide resolved
p2p/conn/connection.go Outdated Show resolved Hide resolved
p2p/conn/secret_connection.go Outdated Show resolved Hide resolved
p2p/pex/addrbook.go Outdated Show resolved Hide resolved
p2p/switch.go Outdated Show resolved Hide resolved
@akaladarshi akaladarshi changed the title p2p: export errors chore: export p2p package errors Jan 23, 2024
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale For use by stalebot label Feb 13, 2024
@github-actions github-actions bot removed the stale For use by stalebot label Feb 14, 2024
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale For use by stalebot label Feb 25, 2024
@github-actions github-actions bot removed the stale For use by stalebot label Feb 26, 2024
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale For use by stalebot label Mar 11, 2024
@github-actions github-actions bot removed the stale For use by stalebot label Mar 12, 2024
@andynog andynog added P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably p2p labels Mar 15, 2024
@andynog andynog self-assigned this Mar 15, 2024
@andynog andynog self-requested a review March 15, 2024 20:28
andynog
andynog requested changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@andynog andynog left a comment

Choose a reason for hiding this comment

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

Overall looks good. Made suggestions for improvements or comments about reverting some changes (especially for messages). Ideally some tests could benefit of using the exported errors

p2p/conn/errors.go Outdated Show resolved Hide resolved
p2p/conn/errors.go Outdated Show resolved Hide resolved
p2p/errors.go Outdated Show resolved Hide resolved
p2p/netaddress.go Outdated Show resolved Hide resolved
p2p/pex/pex_reactor.go Outdated Show resolved Hide resolved
p2p/errors.go Outdated Show resolved Hide resolved
p2p/pex/errors.go Outdated Show resolved Hide resolved
p2p/pex/errors.go Outdated Show resolved Hide resolved
p2p/switch.go Outdated Show resolved Hide resolved
p2p/switch.go Outdated Show resolved Hide resolved
andynog
andynog approved these changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@andynog andynog left a comment

Choose a reason for hiding this comment

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

lgtm, fixed code in suggestions and comments

p2p/conn/connection.go Outdated Show resolved Hide resolved
p2p/conn/connection.go Show resolved Hide resolved
p2p/conn/errors.go Outdated Show resolved Hide resolved
p2p/conn/secret_connection.go Show resolved Hide resolved
p2p/conn/secret_connection.go Show resolved Hide resolved
p2p/pex/errors.go Outdated Show resolved Hide resolved
p2p/pex/errors.go Outdated Show resolved Hide resolved
p2p/switch.go Outdated Show resolved Hide resolved
p2p/switch.go Outdated Show resolved Hide resolved
p2p/pex/pex_reactor.go Outdated Show resolved Hide resolved
@andynog andynog added this pull request to the merge queue Mar 18, 2024
Merged via the queue into cometbft:main with commit c3f7ea3 Mar 18, 2024
22 checks passed
@akaladarshi akaladarshi deleted the tikaryan/export-p2p-errors branch March 19, 2024 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andynog andynog andynog approved these changes

@melekes melekes Awaiting requested review from melekes

@cason cason Awaiting requested review from cason

Assignees

@andynog andynog

Labels
P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably p2p
Projects
CometBFT
Status: Done
Milestone
2024-Q1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@akaladarshi @cason @melekes @andynog @adizere