Conversation
There was a problem hiding this comment.
Please don't change the nuspec file or include packages you have generated. Please do not downgrade the framework version. Also see my comments about your http client changes. Even though the old was using .result it still allowed multiple clients to be created. I would advise you to split your generate invoice pdf change from the base proxy change. As that will be easier to approve.
| </runtime> | ||
|
|
||
| </configuration> No newline at end of file | ||
| <startup><supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.6.1" /></startup></configuration> |
There was a problem hiding this comment.
Because I needed to use it in a project already including a higher version of Newtonsoft.Json than 7
| <RootNamespace>Saasu.API.Client.IntegrationTests</RootNamespace> | ||
| <AssemblyName>Saasu.API.Client.IntegrationTests</AssemblyName> | ||
| <TargetFrameworkVersion>v4.7.1</TargetFrameworkVersion> | ||
| <TargetFrameworkVersion>v4.6.1</TargetFrameworkVersion> |
There was a problem hiding this comment.
Why are you changing the target framework?
There was a problem hiding this comment.
My project was 4.6.1, and I couln't otherwise use the library. Upgrading my framework version led to other issues. I made this change before I intended the pull request.
There was a problem hiding this comment.
PS. It's not a downgrade. Nuget packages should target the lowest possible framework that supports it, so it can be used as widely as possible. Packages targeted for a lower version framework can be used in a project using a higher version framework, but packages with a higher version framework can't be used in a project with a lower version.
| <packages> | ||
| <package id="Microsoft.AspNet.WebApi.Client" version="5.2.3" targetFramework="net471" /> | ||
| <package id="Newtonsoft.Json" version="7.0.1" targetFramework="net471" /> | ||
| <package id="Newtonsoft.Json" version="10.0.3" targetFramework="net461" /> |
There was a problem hiding this comment.
Why are you downgrading the target framework?
| <Saasu.API.Client.Config> | ||
| <setting name="IgnoreSSLErrors" serializeAs="String"> | ||
| <value>false</value> | ||
| <value>true</value> |
There was a problem hiding this comment.
Why was this value changed?
There was a problem hiding this comment.
I'm getting connection SSL errors, though likely because I can't create an outbound connection. The HTTPClient stops working after a few hours after launch. This wasn't intended as part of the pull request.
| : new JsonMediaTypeFormatter()); | ||
| } | ||
|
|
||
| return client.SendAsync(request).Result; |
There was a problem hiding this comment.
This is going to block the call and it won't be async. These changes will only allow a single http request at a time, where before multiple would have been supported.
There was a problem hiding this comment.
No, I don't think so. HttpClient is intended to be shared, and is reentrant and thread safe. Unless I'm missing something.
See https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/
| <RootNamespace>Saasu.API.Client</RootNamespace> | ||
| <AssemblyName>Saasu.API.Client</AssemblyName> | ||
| <TargetFrameworkVersion>v4.7.1</TargetFrameworkVersion> | ||
| <TargetFrameworkVersion>v4.6</TargetFrameworkVersion> |
| <id>Saasu.API.Dotnet.Sdk</id> | ||
| <title>API SDK for the Saasu API</title> | ||
| <version>1.1.7</version> | ||
| <version>1.1.7.2</version> |
There was a problem hiding this comment.
Please don't change the nuspec file. We will do this for a release.
There was a problem hiding this comment.
This commit was not intended to be a part of the pull request. Because there was no response to our request, we made this change internally so we could push to our private nuget repository without conflicts.
| <version>1.1.7.2</version> | ||
| <authors>Saasu</authors> | ||
| <description>A client SDK for working with the Saasu API.</description> | ||
| <description>A client SDK for working with the Saasu API. Modified for Intellis internal use.</description> |
There was a problem hiding this comment.
This is not an appropriate description.
There was a problem hiding this comment.
As above. This was not intended to be pulled to the base repo
| <packages> | ||
| <package id="Microsoft.AspNet.WebApi.Client" version="5.2.3" targetFramework="net471" /> | ||
| <package id="Newtonsoft.Json" version="7.0.1" targetFramework="net471" /> | ||
| <package id="Newtonsoft.Json" version="10.0.3" targetFramework="net46" /> |
2 participants
Hi,
I've included some changes here that we've made internally.