search

oxidecomputer / dropshot

expose REST APIs from a Rust program
Apache License 2.0
843 stars 74 forks source link

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

Closed sunshowers closed 3 months ago

sunshowers commented 4 months ago

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.

ahl commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

sunshowers commented 3 months ago

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.

Ended up changing these tests to be a new kind of "server-only" tests, so renaming them wasn't too bad -- did that in this PR.