Remove Microsoft.AspNetCore.WebUtilities dependency #419
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
/assign @krabhishek8260 |
| <PackageReference Include="Microsoft.Rest.ClientRuntime" Version="2.3.10" /> | ||
| <PackageReference Include="Newtonsoft.Json" Version="9.0.1" Condition="'$(TargetFramework)' != 'netstandard2.0' and '$(TargetFramework)' != 'netcoreapp2.1'" /> | ||
| <PackageReference Include="Newtonsoft.Json" Version="12.0.2" Condition="'$(TargetFramework)' == 'netstandard2.0' or '$(TargetFramework)' == 'netcoreapp2.1'" /> | ||
| <PackageReference Include="YamlDotNet" Version="6.0.0" /> | ||
| <PackageReference Include="System.Buffers" Version="4.5.1" Condition="'$(TargetFramework)' != 'netcoreapp2.1'" /> |
There was a problem hiding this comment.
This is one of the indirect dependencies pulled in by Microsoft.AspNetCore.WebUtilities, and it is being used.
|
Please add a unit test for the query string generator then this is ready to merge. |
| { "stdout", stdout ? "1": "0"}, | ||
| { "tty", tty ? "1": "0"} | ||
| }).TrimStart('?'); | ||
| var query = new StringBuilder(); |
There was a problem hiding this comment.
I would prefer not to manipulate strings
Maybe copy the code from WebUtilities?
QueryHelpers is more readable
There was a problem hiding this comment.
How about if I:
- Call Utilities.AddQueryParameter for each line rather than using Append, or
- Add Utilities.AddQueryParameters which can taken multiple parameters, and use that?
@brendandburns I tried to but I want to stop - or at least not contribute to - the exposure of implementation details publicly which I think is hampering changes to the client by making changes breaking when they really shouldn't be. Is there some way to share internal types with the tests? Normally you use the [InternalsVisibleTo] attribute, but that requires a signed assembly. I remember seeing some comments about this in the past but I don't know where that went... |
|
@brendandburns Okay, I added a test, which involved signing the test assembly so it can access the internals of the main client assembly. Hopefully future implementation details can remain "internal" so they can change without needing a breaking API change. |
|
link #421 |
admilazz
force-pushed
the
removeWebUtilities
branch
from
5daed62 to
4a440f2
Compare
Seems we made the same change at the same time. I merged in yours. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: admilazz, brendandburns The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
This change removes the Microsoft.AspNetCore.WebUtilities dependency (which pulls in several other unnecessary dependencies as well). The library was only used to manipulate query strings, but that can be done simply without it. Some minor bugs found in query-string handling were also fixed.