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

multi: update wire error types. #2267

Closed
wants to merge 1 commit into from

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Jul 16, 2020

This updates the wire error types to leverage go 1.13 errors.Is/As functionality as well as confirm to the error infrastructure best practices outlined in #2181.

@davecgh
Copy link
Member

davecgh commented Jul 16, 2020

Similarly, there are multiple types of errors that can come out of the wire package, so I really don't like renaming it to something generic that loses that extra information.

@davecgh davecgh added this to the 1.7.0 milestone Jul 17, 2020
@dnldd dnldd force-pushed the update_wire_error_types branch 2 times, most recently from 64e3766 to c69f6ba Compare October 14, 2020 17:31
This updates the wire error types to leverage go
1.13 errors.Is/As functionality as well as confirm to
the error infrastructure best practices.
@davecgh
Copy link
Member

davecgh commented Oct 20, 2020

This is going to be a pretty big hassle to implement to be honest. It needs a major module bump because it changes the API in an incompatible way. The wire package is also pretty much used in the API of every single other package which means all of their module majors will need to be bumped too.

I'm not convinced it's worth it to be honest. However, if you really want to get this one, it'll need the standard set of PRs to start the development cycle for the new major version, etc.

@davecgh
Copy link
Member

davecgh commented Dec 18, 2020

I'm going to hold off on this one for a while until we really need a new major wire version as a result of changing the primitives out.

I expect that will happen either late in the 1.7.0 dev cycle or for 1.8.0. So, I'm going to mark this one as 1.8.0 for now to set expectations properly.

@davecgh davecgh modified the milestones: 1.7.0, 1.8.0 Dec 18, 2020
@davecgh
Copy link
Member

davecgh commented Jan 14, 2022

Now that we're in the 1.8.0 development cycle, we should decide about working towards getting this in.

It requires a major module bump and that will require major module bumps of a bunch of other modules that use the types in their public APIs. I believe that will include addrmgr, blockchain, blockchain/standalone, blockchain/stake, connmgr, database, dcrutil, gcs, peer, rpcclient, and txscript.

Given it will necessarily cause so many major module version bumps, I wonder if we should just hold off until primitives (#2786) is ready to go because it too will require major bumps to all of the same modules. However, I don't foresee that module being done for quite some time, and almost positively not within the 1.8.0 time frame.

@dnldd
Copy link
Member Author

dnldd commented Jan 24, 2022

As discussed in chat, I think we are better off bundling this with the changes coming in #2786 since this isn't priority and would require similar amounts of work to get in.

@davecgh davecgh closed this Apr 11, 2023
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.

None yet

2 participants