Skip to content
This repository has been archived by the owner on Dec 18, 2017. It is now read-only.

fix #1113 by adding 'dnu feeds list' command #2121

Merged
merged 1 commit into from
Jun 30, 2015

Conversation

analogrelay
Copy link
Contributor

Fixes #1113

Add dnu sources command that lists active sources. It looks like this:

image

Disabled sources have [Disabled] next to them in yellow to identify them as disabled. I also tried to centralize some of the disjointed code we have to handle NuGet.config.

/cc @davidfowl @victorhurdugaci @troydai


[Theory]
[MemberData(nameof(RuntimeComponents))]
public void DnuSources_ListsAllSources(string flavor, string os, string architecture)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% happy with this test. It's a little too brittle. I might see if I can tidy it up before pushing.

@victorhurdugaci
Copy link
Contributor

Oh no! Please don't add sources. Change the name to something else. I already have a PR for dnu source which is for bringing source code :)


namespace Microsoft.Framework.PackageManager
{
internal static class SourcesCommand
Copy link
Contributor

Choose a reason for hiding this comment

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

SourcesConsoleCommand

@analogrelay
Copy link
Contributor Author

Ok, but sources aligns with nuget so we might need to consider all the options...

@analogrelay
Copy link
Contributor Author

I'm not sure I like source for the command you're building, it collides with the concept of package sources. I would propose something more like dnu clone for that command. It seems clearer what it does and frees this up.

I'm definitely open to renaming this though if people think that would be less confusing. We could do something like dnu list-sources (which does start to bring up potential collision with dnu list)

@@ -0,0 +1,44 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

License.

@troydai
Copy link
Contributor

troydai commented Jun 24, 2015

else
{
Reports.Information.WriteLine("Registered Sources:");
// Iterate over the sources and report them
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: add empty line before comment

@BrennanConroy
Copy link
Member

Fix tests!

@analogrelay
Copy link
Contributor Author

I'm going to rename this to list-sources for now to unblock. I'll also go through the CI failures and remaining code review feedback.

@analogrelay
Copy link
Contributor Author

Ping @troydai @BrennanConroy @victorhurdugaci - Updated to resolve feedback.

{
internal class ListSourcesCommand
{
public Reports Reports { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be required and if it null it will fail. Why don't you pass it as a constructor arg ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just because everything else did it this way :). I'll fix this one

@analogrelay
Copy link
Contributor Author

Ping @victorhurdugaci again :)

@analogrelay analogrelay changed the title fix #1113 by adding 'dnu sources' command fix #1113 by adding 'dnu feeds list' command Jun 29, 2015
@troydai
Copy link
Contributor

troydai commented Jun 29, 2015

My concerns are addressed. :shipit:

@analogrelay
Copy link
Contributor Author

I can't figure out what's wrong with the PR builds. I'm going to give a private TeamCity build a quick shot but if I can't work that out, just YOLO push and be prepared to revert :).

@analogrelay analogrelay merged commit 36eab9f into dev Jun 30, 2015
@analogrelay analogrelay deleted the anurse/1113-dnu-sources branch August 3, 2015 18:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants