Skip to content

Move error codes to message descriptions, include newer error messages #656

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

Merged
merged 145 commits into from
Feb 6, 2025

Conversation

martinduke
Copy link
Contributor

@martinduke martinduke commented Jan 21, 2025

The error codes are currently in the "Relays" section, which doesn't make any sense.

It's also missing codes for SUBSCRIBE_ANNOUNCES_ERROR, ANNOUNCE_ERROR, and FETCH_ERROR.

Also fixes #577.

@martinduke martinduke requested review from afrind and removed request for vasilvv, kixelated and suhasHere January 21, 2025 21:57
Copy link
Collaborator

@afrind afrind 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 doing this. Do we need to look at Track Status responses too?

@martinduke
Copy link
Contributor Author

Thank you for doing this. Do we need to look at Track Status responses too?

I don't want to take that on in this PR. These are mostly obvious tweaks from SUBSCRIBE_ERROR to *_ERROR.

Copy link
Collaborator

@suhasHere suhasHere left a comment

Choose a reason for hiding this comment

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

this seems ok , but rather duplicated ..

Copy link
Collaborator

@afrind afrind left a comment

Choose a reason for hiding this comment

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

Individual Comments

@martinduke
Copy link
Contributor Author

this seems ok , but rather duplicated ..

Agreed, but each of the messages has a couple of slightly different error conditions, unfortunately.

@martinduke martinduke self-assigned this Jan 29, 2025
Copy link
Collaborator

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

LG once you update the name and fix the merge conflicts.

This reverts commit 85d6fd7.
This reverts commit 4c1c05d.
This reverts commit 4655de8.
This reverts commit fab5a7c.
This reverts commit 32b22a6.
This reverts commit 1888ad5.
This reverts commit 7e78c15.
@martinduke
Copy link
Contributor Author

I guess this is done, although git, as usual, is being stupid. I don't think any errors crept in, but I'm not sure.

Copy link
Collaborator

@fluffy fluffy left a comment

Choose a reason for hiding this comment

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

I assume this is 100% editorial but LGTM.

@ianswett ianswett merged commit 773a241 into main Feb 6, 2025
2 checks passed
@englishm englishm mentioned this pull request Feb 25, 2025
39 tasks
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.

Errors from Announce
7 participants