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

api: FromConn for serde_json::Value and querystrong::QueryStrong #359

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jbr
Copy link
Contributor

@jbr jbr commented Aug 15, 2023

No description provided.

@jbr jbr force-pushed the api-querystrong branch from b08518f to 8884f1f Compare August 15, 2023 22:27
@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #359 (8884f1f) into main (b98e4ca) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #359      +/-   ##
==========================================
- Coverage   45.25%   45.20%   -0.05%     
==========================================
  Files         156      156              
  Lines        6057     6063       +6     
==========================================
  Hits         2741     2741              
- Misses       3316     3322       +6     
Files Changed Coverage Δ
api/src/from_conn.rs 15.38% <0.00%> (-9.62%) ⬇️

... and 1 file with indirect coverage changes

@joshtriplett
Copy link
Collaborator

I just encountered a case where it would have been nice to have support for QueryStrong.

@jbr
Copy link
Contributor Author

jbr commented Jan 30, 2024

I just encountered a case where it would have been nice to have support for QueryStrong.

The reason I haven't moved more decisively on this is until there can be a const generic str, the FromConn will really just be QueryStrong::from_str(conn.querystring()).unwrap_or_default() which isn't a lot of saved characters for adding a dependency. When/if const str is a thing, we could have:

fn api_handler(conn: &mut Conn, query: Query<Path = "name[first]">) {}

or whatever that syntax eventually looks like, where Path is the string representation of a querystrong::Indexer

Convince me it's worth the dependency and cargo feature to make QueryStrong::from_str(conn.querystring()).unwrap_or_default() easier? I've also made similar initial steps towards adding this to trillium itself, but some people may prefer to use serde_qs or serde_querystring. Should trillium itself have a querystrong feature? Or just trillium-api? If either, should the feature be enabled by default?

@joshtriplett
Copy link
Collaborator

joshtriplett commented Jan 31, 2024

@jbr I don't think it's worth the slight brevity in getting the entire query string. However, I think it'd be worth it if you could write something like Query<SomeStruct> to deserialize a struct from the query string.

Separate from that, I'd also love to have simple support for cases like xyz?value=:value that feels more-or-less integrated with routing, making it possible to extract :value in the same manner as path components. Ideally, that'd work in any order, and provide a way to declare them optional or required. (Much like current routing, trillium-api can't currently extract those parameters by name because of the lack of const generic strings, but I built a local mechanism to extract parameters whose name is a number, using const generic numbers. :) )

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