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

[4/n] [dropshot-endpoint] unify error message style #1009

Open
wants to merge 9 commits into
base: sunshowers/spr/main.dropshot-endpoint-unify-error-message-style
Choose a base branch
from

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented May 15, 2024

Always use messages of the form "endpoint {name_str} ...", without a period
at the end. The casing and lack of period at the end matches rustc's style.

One difference from that style is that we now include the name of the endpoint
in error messages. That's particularly important for trait-based Dropshot
servers, where in some cases we might end up losing span info and producing
messages that don't include which endpoint they're talking about. Include the
name of the endpoint unconditionally for a unified style, and so that we don't
have to have unnecessary conditionals. (I personally found having the endpoint
name to be friendlier anyway.)

Depends on #1008.

Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
@sunshowers sunshowers changed the title [dropshot-endpoint] unify error message style [4/n] [dropshot-endpoint] unify error message style May 16, 2024
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
@ahl
Copy link
Collaborator

ahl commented May 31, 2024

From the readme you added:

Some of these tests may be mirrored as multiple closely-related tests. For example, endpoints require that at least one argument is present, so bad_endpoint1.rs tests the situation where no arguments have been provided. But channels require that at least two arguments are present, so we have bad_channel1a.rs, which passes in zero arguments, and bad_channel1b.rs, which passes in one argument.

When are we adding lettered endpoint tests? Can we rename them and/or update the readme?

@sunshowers
Copy link
Contributor Author

When are we adding lettered endpoint tests? Can we rename them and/or update the readme?

Hah so this is actually anticipating some future work -- in particular, the four tests 21a/b/c/d are all broken out from a single test_server_endpoint test. Suggestions on rewording this?

@ahl
Copy link
Collaborator

ahl commented Jun 6, 2024

When are we adding lettered endpoint tests? Can we rename them and/or update the readme?

Hah so this is actually anticipating some future work -- in particular, the four tests 21a/b/c/d are all broken out from a single test_server_endpoint test. Suggestions on rewording this?

I think just renumber? I mostly worry about folks being unclear on what pattern to follow in the future. It doesn't matter, but I think it would be good to limit the number of patterns one might see.

@sunshowers
Copy link
Contributor Author

I think just renumber? I mostly worry about folks being unclear on what pattern to follow in the future. It doesn't matter, but I think it would be good to limit the number of patterns one might see.

Hm ok, I think I'll do that in a followup if that's okay? I have around 20 tests with higher numbers and would have to renumber all of them.

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