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

AppendUrlQueryPairs & QueryPairsExtension traits #266

Merged
merged 5 commits into from
Aug 18, 2024
Merged

Conversation

oguzkocer
Copy link
Contributor

@oguzkocer oguzkocer commented Aug 16, 2024

This PR implements a more structured way to generate the url query for our Params types. This helps avoid duplicating the same logic for these types and makes it less error prone. It's both easier to work with and easier to read.

For the AppendUrlQueryPairs implementation, I've opted for the fn append_query_pairs(&self, query_pairs_mut: &mut QueryPairs) {} function signature. In general, I'd prefer us to return data as oppose to mutating given data. However, in this case, that meant that we had to clone some of the fields. That's probably OK for the gained ergonomics, but I don't feel comfortable with just probably. There is also no practical downside in this case and it sort of works like a unit function because we start with a completely empty query. Another option could be to return a form_urlencoded::Serializer type, then turn that into &str and pass it into url::Url::set_query, but we would still pay an extra cost.

It's also possible I might have missed an approach that can return an IntoIterator, but I don't think it's worth spending too much time on it when we have an acceptable implementation.

@oguzkocer oguzkocer added the Rust label Aug 16, 2024
@oguzkocer oguzkocer added this to the 0.2 milestone Aug 16, 2024
@oguzkocer oguzkocer changed the title AppendUrlQueryPairs & QueryPairsExtension AppendUrlQueryPairs & QueryPairsExtension traits Aug 16, 2024
@oguzkocer oguzkocer marked this pull request as ready for review August 16, 2024 18:26
@oguzkocer oguzkocer enabled auto-merge (squash) August 16, 2024 18:26
@@ -73,6 +75,8 @@ pub enum WpApiParamOrder {
Desc,
}

impl_as_query_value_from_as_str!(WpApiParamOrder);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to implement a #[derive(AsQueryValue)] to do the same? This is not a request for changes though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's, but the implementation would be more complicated because if we are going to have a single derive such as AsQueryValue, then we'd need to have options for it, such as from=as_str, from=to_string etc. Handling that is more complicated, so it add maintenance burden. We could add different derives such as AsQueryValueFromAsStr, AsQueryValueFromToString, but that's not any better unfortunately.

We are trying to implement derive macros when there is significant benefit to them compared to other solutions. This is not such a case, because the simple macro_rules! does a good job and it is easier to understand.

Fwiw, for a personal project I'd probably go with the derive macro as I am used to working with them. However, I don't think it's the right choice for this project - at least not at this time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation!

@oguzkocer oguzkocer merged commit d816844 into trunk Aug 18, 2024
22 checks passed
@oguzkocer oguzkocer deleted the url-query-value branch August 18, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants