8000 Add MethodInfo property by donnytian · Pull Request #1219 · reactiveui/refit · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add MethodInfo property #1219

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Add MethodInfo property #1219

wants to merge 2 commits into from

Conversation

donnytian
Copy link
@donnytian donnytian commented Aug 9, 2021 •
edited
Loading

What kind of change does this PR introduce?
feature

What is the current behavior?
As #1150 and #1156 described, currently, we are not able to get information on the executing method. The information is useful for url tags, additional behavior based on custom attributes, and other things.

What is the new behavior?
Added MethodInfo of the current method into request properties.
Now you can grab the custom attributes and o 8000 ther method info from the request properties in DelegatingHandler.

    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        // Get the methodInfo of the current method
        var methodInfo = (MethodInfo)request.Properties[HttpMessageRequestOptions.MethodInfo];

        // do something with methodInfo with method name, custom attributes and etc.

        return await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
    }

What might this PR break?
There is nothing to break. Just one additional item in the request property/option list.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:
NA

@dnfadmin
Copy link
dnfadmin commented Aug 9, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

@RyuzakiH
Copy link

Can someone review this? It's not that big, but it makes things easier.

@clairernovotny
Copy link
Member

I discussed this with @anaisbetts and we agreed that the type being stored in the dictionary should be a string with the method's name in it. If you can update the PR to do that, I can merge it in.

@donnytian
Copy link
Author

I discussed this with @anaisbetts and we agreed that the type being stored in the dictionary should be a string with the method's name in it. If you can update the PR to do that, I can merge it in.

Thanks for your reply!
Just one question here, we already have the interface stored in the dictionary as Type, can you please explain why we are going to store a string for the method?
Because methods can have overloads with the same name. A simple method name string does not help unless we format the name and the types of all arguments (but why bother that, consumer has to decode the signature string back).

@netssfy
Copy link
netssfy commented Nov 4, 2021

I think strong type is reasonable, because refit http attributes are applied on the method which is a strong type.
So I also want to know why @anaisbetts choose storing a string with method name?

@james-s-tayler
Copy link
Contributor

Seems this is an issue people seem to have a difference of opinion hence why I liked my configurable approach... :(

@donnytian
Copy link
Author

Seems this is an issue people seem to have a difference of opinion hence why I liked my configurable approach... :(

I love it too. I love either of them to be merged more. Our project currently is using an ugly&complex workaround to just get the executing method...

@0xced
Copy link
Contributor
0xced commented Feb 15, 2022

I just opened #1317 which is very closely related but goes a little bit further by exposing the full RestMethodInfo and also provides new HttpRequestMessageOptions.InterfaceTypeKey and HttpRequestMessageOptions.RestMethodInfoKey properties to ease retrieving the request options.

But I should definitively have looked at open pull requests first!

@0xced
Copy link
Contributor
0xced commented Feb 15, 2022

Seems this is an issue people seem to have a difference of opinion hence why I liked my configurable approach... :(

If I understand correctly, you are talking about #1180, am I right?

@james-s-tayler james-s-tayler mentioned this pull request May 28, 2022
Merged
2 tasks
@gabrielmaldi
Copy link

I would love to have this to be able to override HttpClient's timeout on a per-operation basis:

services
    .AddRefitClient()
    .ConfigureHttpClient(httpClient => httpClient.Timeout = TimeSpan.FromSeconds(30));
public class HttpMessageHandler : DelegatingHandler
{
    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        var timeoutSeconds = 15; // ToDo: read configuration for operation identified by request.Options[HttpRequestMessageOptions.MethodInfo]
        using var timeoutTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(timeoutSeconds));
        return await base.SendAsync(request, timeoutTokenSource.Token);
    }
}

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@donnytian @dnfadmin @RyuzakiH @clairernovotny @netssfy @james-s-tayler @0xced @gabrielmaldi @glennawatson
0