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

Support providing the HttpClient direct to the FhirClient #2049

Merged

Conversation

brianpos
Copy link
Collaborator

Description

Support providing the HttpClient direct to the FhirClient instead of the HttpClientHandler (as another alternative)

This then supports creating unit tests as described here for webapi based projects
https://www.hanselman.com/blog/minimal-apis-in-net-6-but-where-are-the-unit-tests

Related issues

Resolves/Contributes to #2046 #2036 (may be able to now push the HttpClientFactory work to the callers, and not be required in the FirelySDK at all)

Testing

Additional unit test included (and also included in my server to verify that can unit test as shown)

…just the HttpClientHandler (as another alternative)

This then supports creating unit tests as described here for webapi based projects
https://www.hanselman.com/blog/minimal-apis-in-net-6-but-where-are-the-unit-tests
Copy link
Member

@marcovisserFurore marcovisserFurore left a comment

Choose a reason for hiding this comment

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

I am not sure we are going to pull this for the next release, but for now these are my comments:

src/Hl7.Fhir.Core/Rest/FhirClient.cs Outdated Show resolved Hide resolved
@brianpos
Copy link
Collaborator Author

Made the changes as requested @marcovisserFurore

Copy link
Member

@marcovisserFurore marcovisserFurore left a comment

Choose a reason for hiding this comment

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

Some documentation adjustments needed

/// If the endpoint does not end with a slash (/), it will be added.
/// </summary>
/// <remarks>
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
/// If the httpClient is provided then it must be disposed by the caller
Suggested change
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller

/// </summary>
/// <remarks>
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
/// Only one of the messageHandler or httpClient can be provided
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Only one of the messageHandler or httpClient can be provided
Suggested change
/// Only one of the messageHandler or httpClient can be provided
/// Only one of the messageHandler or httpClient can be provided

/// </summary>
/// <remarks>
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
/// Only one of the messageHandler or httpClient can be provided
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Only one of the messageHandler or httpClient can be provided
Suggested change
/// Only one of the messageHandler or httpClient can be provided
/// Only one of the messageHandler or httpClient can be provided

#pragma warning disable CS0067

/// <summary>
/// Creates a new client using a default endpoint
/// If the endpoint does not end with a slash (/), it will be added.
/// </summary>
/// <remarks>
/// If the messageHandler is provided then it must be disposed by the caller
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
/// If the messageHandler is provided then it must be disposed by the caller
Suggested change
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller

#pragma warning disable CS0067

/// <summary>
/// Creates a new client using a default endpoint
/// If the endpoint does not end with a slash (/), it will be added.
/// </summary>
/// <remarks>
/// If the messageHandler is provided then it must be disposed by the caller
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
/// If the messageHandler is provided then it must be disposed by the caller
Suggested change
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller

#pragma warning disable CS0067

/// <summary>
/// Creates a new client using a default endpoint
/// If the endpoint does not end with a slash (/), it will be added.
/// </summary>
/// <remarks>
/// If the messageHandler is provided then it must be disposed by the caller
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
/// Only one of the messageHandler or httpClient can be provided
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Only one of the messageHandler or httpClient can be provided
Suggested change
/// Only one of the messageHandler or httpClient can be provided
/// Only one of the messageHandler or httpClient can be provided

src/Hl7.Fhir.Core/Rest/FhirClient.cs Outdated Show resolved Hide resolved
/// If the endpoint does not end with a slash (/), it will be added.
/// </summary>
/// <remarks>
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
/// If the httpClient is provided then it must be disposed by the caller
Suggested change
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller
/// If the messageHandler, or httpClient is provided then it must be disposed by the caller

@marcovisserFurore marcovisserFurore merged commit 1ed7174 into FirelyTeam:develop-r4 Apr 26, 2022
@brianpos brianpos deleted the feature/BP-2029-httpclient branch May 23, 2022 05:49
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.

None yet

2 participants