Skip to content

[14/n] [dropshot_endpoint] support tag_config in API traits#1059

Merged
sunshowers merged 12 commits into
mainfrom
sunshowers/spr/dropshot_endpoint-support-tag_config-in-api_description
Aug 15, 2024
Merged

[14/n] [dropshot_endpoint] support tag_config in API traits#1059
sunshowers merged 12 commits into
mainfrom
sunshowers/spr/dropshot_endpoint-support-tag_config-in-api_description

Conversation

@sunshowers

Copy link
Copy Markdown
Contributor

Based on today's discussion, we're supporting a static tag_config argument to
api_description. The implementation just creates a dropshot::TagConfig and
sets that.

Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1

[skip ci]
@sunshowers sunshowers changed the title [dropshot_endpoint] support tag_config in api_description [dropshot_endpoint] support tag_config in API traits Jul 9, 2024
@sunshowers sunshowers changed the title [dropshot_endpoint] support tag_config in API traits [14/n] [dropshot_endpoint] support tag_config in API traits Jul 9, 2024
@sunshowers sunshowers requested a review from ahl July 9, 2024 08:35
Created using spr 1.3.6-beta.1
Comment thread dropshot_endpoint/src/api_trait.rs Outdated
Comment thread dropshot_endpoint/src/api_trait.rs Outdated
Comment thread dropshot_endpoint/src/lib.rs Outdated
Comment thread dropshot_endpoint/src/lib.rs Outdated
Comment thread dropshot_endpoint/src/lib.rs Outdated
Comment thread dropshot_endpoint/src/lib.rs Outdated
Comment thread dropshot_endpoint/src/api_trait.rs Outdated
/// A mirror of dropshot's `EndpointTagPolicy`, used as part of arguments to the
/// top-level `api_description` macro.
#[derive(Clone, Copy, Default, Deserialize, Debug)]
#[serde(rename_all = "snake_case")]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

are there other places where we use snake_case in a similar setting?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't think so -- we use snake_case for keys but don't have any other places where enum variants are tagged as values. snake_case looks more correct to me here though, do you think otherwise?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess I don't have an opinion. It seemed odd to me... based on my knowledge of the implementation of serde_tokenstream, not as an end-user. Maybe show it to a couple of unbiased people and see if they have a preference?

@sunshowers sunshowers Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ran a poll on Mastodon which overwhelmingly agreed with you: https://hachyderm.io/@rain/112832705113530863

But one suggestion was to use EndpointTagPolicy::AtLeastOne, i.e. the actual value in there. I like that a lot. I think that should be possible with ParseWrapper, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Implemented this and I really like it, but at what cost: oxidecomputer/serde_tokenstream#195

Comment thread dropshot_endpoint/src/lib.rs Outdated
Comment thread dropshot_endpoint/src/lib.rs Outdated
Comment thread dropshot_endpoint/src/lib.rs
Created using spr 1.3.6-beta.1

[skip ci]
Created using spr 1.3.6-beta.1
@sunshowers sunshowers changed the base branch from sunshowers/spr/main.dropshot_endpoint-support-tag_config-in-api_description to main July 12, 2024 18:33
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
@sunshowers sunshowers merged commit ea76a35 into main Aug 15, 2024
@sunshowers sunshowers deleted the sunshowers/spr/dropshot_endpoint-support-tag_config-in-api_description branch August 15, 2024 01:16
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.

2 participants