(GH-143) Make NuGet calls yield return, so we stream output as it comes #224
Conversation
|
Does this supersede #146 ? |
|
Yeah, I guess so! I had totally forgotten that pull request was still open, so I basically rebased everything I had which wasn't already merged, and then submitted them. They are in order, each one should stack, and we can forget about #146 |
|
What are we waiting for on this, @ferventcoder? |
| @@ -412,7 +414,7 @@ public void should_call_nuget_service_list_run_when_command_is_list() | |||
| configuration.PinCommand.Command = PinCommandType.list; | |||
| command.run(configuration); | |||
|
|
|||
| nugetService.Verify(n => n.list_run(It.IsAny<ChocolateyConfiguration>(), false), Times.Once); | |||
| nugetService.Verify(n => n.list_run(It.IsAny<ChocolateyConfiguration>(), true), Times.Once); | |||
There was a problem hiding this comment.
same question here. What changed this from false to true?
There was a problem hiding this comment.
Same answer: The test is "if I call X does Y get called" ....
After the changes to make things stream output, you have to set this to TRUE so the results would get enumerated (and the method under test would get called). Of course, this parameter goes away in the next commit, because we got rid of the boolean and moved it to the super-all-knowing-config-object.
|
Perhaps we should add a switch to the API (config) like |
|
@Jaykul yes perhaps. I want to get this thing moving along. I just want to make sure we test all avenues. I think we'll want to add an integration spec for each scenario and see what happens. |
|
👍 for getting this moving along ;) |
| @@ -896,13 +891,11 @@ private void set_package_names_if_all_is_specified(ChocolateyConfiguration confi | |||
| var input = config.Input; | |||
| config.Input = string.Empty; | |||
|
|
|||
| var localPackages = list_run(config, logResults: false); | |||
|
|
|||
| config.PackageNames = list_run(config, false).Select(p => p.Name).@join(ApplicationParameters.PackageNamesSeparator); | |||
|
Okay, this is completely weird. Let's look at the two scenarios we have. One is with For memory/time consumption, I ran against the full list from chocolatey.org. I had no noticeable memory issues locally (if that's what you were getting at with memory consumption, but I was only looking at CPU consumption in whole percentages). The actual impact of time was negligent. Both took around 20 seconds total to output the results. It was about 2 seconds slower to implement OrderBy (but I didn't see it as a blocking operation, unsure why not). However the number of packages was different - I came up with 2780-ish packages with NuGet.Commandline adjustments and only 2172-ish with the PR as is. The time to first result from query was also kind of interesting. In the NuGet.CommandLine scenario, it's about 500 milliseconds to first result. In the original PR scenario, it doesn't appear to stream appropriately. I'm guessing the difference here has to do with logging the output. @Jaykul what kind of results where you seeing? |
| @@ -80,7 +80,7 @@ public void list_run(ChocolateyConfiguration config, bool logResults) | |||
| var list = _nugetService.list_run(config, logResults: true); | |||
| if (config.RegularOutput) | |||
| { | |||
| this.Log().Warn(() => @"{0} packages {1}.".format_with(list.Count, config.ListCommand.LocalOnly ? "installed" : "found")); | |||
Fix NuGet so it streams output.
WARNING NOTE: This makes it VERY IMPORTANT that SOMEONE must enumerate the results, or NOTHING will happen. ;-)
Closes #143.