#1107 Do no set default visibility and affiliation values to avoid mixing with type

[AlexP11223]

An attempt to fix mixing of visibility/affiliation and type described in #1107.

[M-Zuber]

LGTM, but I would say there should be a check/manual set to default for the type parameter if either visibility or affiliation are set.
just my 2¢

[ryangribble]

With this change, won't there still be an issue if you set a non Default value for visibility or affiliation, since the Type property still has a value specified that will cause a parameter item to be sent through?

So you'd either have to add your ParameterValue("")Default option to Type as well or perhaps another approach is to make the enum properties on RepositoryRequest be nullable, that way when they arent specified they wont set anything in the parameter array?

eg

public class RepositoryRequest : RequestParameters
{
    public RepositoryType? Type { get; set; }
    public RepositorySort? Sort { get; set; }
    public SortDirection? Direction { get; set; }
    public RepositoryVisibility? Visibility { get; set; }
    public RepositoryAffiliation? Affiliation { get; set; }
}

Whichever way it's done, I agree with @M-Zuber it would be useful to check the RepositoryRequest in GetAllForCurrent() and throw an error if "invalid" combination is specified by the consumer
eg:

if ((request.Visibility != null || request.Affiliation != null) && request.Type != null)
{
    throw new ArgumentException("If you specify Visibility or Affiliation, you cannot specify Type.");
}

Perhaps @shiftkey can advise between the preference of adding a Default enum element (that has a blank property value causing nothing to be specified) vs allowing nullable properties?

Also when I'm adding functionality to Octokit, I always try to add tests as well, so that from now on we can assert that this problem never re-appears down the track.

In this case I would add unit tests to cover:

• RepositoryRequest - when null (in my example) or .Default (in your example) is specified for each property, that corresponding parameter name does not appear in the parameter dictionary created by request.ToParametersDictionary()
• RepositoriesClient.GetAllForCurrent() throws the expected exception when incorrect combination of flags is provided

[shiftkey]

Perhaps @shiftkey can advise between the preference of adding a Default enum element (that has a blank property value causing nothing to be specified) vs allowing nullable properties?

I'd rather these be optional parameters as that's the precedence we have for handling values that may be set. And as per the docs, there's documented default behaviour here for all these values if they're not set:

I have some concerns that I should have raised earlier. Consider this scenario:

var request = new RepositoryRequest
{
   Visibility = RepositoryVisibility.Public,
   Type = RepositoryType.Owner
};

This is a valid object to create, however it will fail during the request. Do we guard against that, or just bubble up the exception to the user? There's an escape hatch we have to override RequestParameters.ToParametersDictionary and verify that a valid set of parameters are provided - and throw otherwise.

Anyway, 👍 to throwing more unit tests into this area - RequestParameters is very important, and I'd hate to introduce something unexpected here.

[AlexP11223]

I made the properties nullable instead of Default enum parameters and added unit tests for RepositoryRequest ToParamatersDictionary.

[ryangribble]

Is this line necessary? The parameter array was still created correctly for me, even without this line...

[AlexP11223]

Without this line it does not go into if (propertyType.IsEnumeration()) and somehow produces "All" instead of "all" + does not use [Parameter] attribute.

[ryangribble]

Ah fair enough, I must admit I just quickly put the nullable properties on the request object and ran the existing integration tests which passed... From what you're saying we might need more tests since the current ones didn't pick up the problem you highlighted... oops, obviously I don't have your new test which looks like it covers those scenarios 😀

[ryangribble]

Nice work on the parameter array unit test :)

While we're here I wonder if we should tweak the DebuggerDisplay() on RepositoryRequest since it doesnt list the Affiliation or Visibility properties plus it outputs field names for the other fields that can now be blank?

[AlexP11223]

Yeah, I was thinking about DebuggerDisplay too yesterday.:)

For null values, shoud we output something like Visibility: <null> or remove the property from the output?

[ryangribble]

For null values, shoud we output something like Visibility: or remove the property from the output?

I think the way you've done it is good (if the values are null, don't mention them at all).

LGTM - I'll just wait for a +1 from @shiftkey before merging

[shiftkey]

Looks good to me! Thanks @AlexP11223!

And thanks @M-Zuber @ryangribble for helping out with the review!