Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions Octokit.Tests/Models/RepositoryRequestTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
using Xunit;

namespace Octokit.Tests.Models
{
public class RepositoryRequestTests
{
public class TheToParametersDictionaryMethod
{
[Fact]
public void ContainsSetValues()
{
var request = new RepositoryRequest
{
Type = RepositoryType.All,
Sort = RepositorySort.FullName,
Direction = SortDirection.Ascending
};

var parameters = request.ToParametersDictionary();

Assert.Equal(3, parameters.Count);
Assert.Equal("all", parameters["type"]);
Assert.Equal("full_name", parameters["sort"]);
Assert.Equal("asc", parameters["direction"]);

request = new RepositoryRequest
{
Affiliation = RepositoryAffiliation.All,
Visibility = RepositoryVisibility.Public
};

parameters = request.ToParametersDictionary();

Assert.Equal(2, parameters.Count);
Assert.Equal("owner, collaborator, organization_member", parameters["affiliation"]);
Assert.Equal("public", parameters["visibility"]);
}

[Fact]
public void DoesNotReturnValuesForDefaultRequest()
{
var request = new RepositoryRequest();

var parameters = request.ToParametersDictionary();

Assert.Empty(parameters);
}
}
}
}
1 change: 1 addition & 0 deletions Octokit.Tests/Octokit.Tests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@
<Compile Include="Models\PullRequestRequestTests.cs" />
<Compile Include="Models\PunchCardTests.cs" />
<Compile Include="Models\ReadOnlyPagedCollectionTests.cs" />
<Compile Include="Models\RepositoryRequestTests.cs" />
<Compile Include="Models\RepositoryUpdateTests.cs" />
<Compile Include="Models\RequestParametersTests.cs" />
<Compile Include="Models\SearchCodeRequestTests.cs" />
Expand Down
10 changes: 5 additions & 5 deletions Octokit/Models/Request/RepositoryRequest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,39 +19,39 @@ public class RepositoryRequest : RequestParameters
/// The type.
/// </value>
[SuppressMessage("Microsoft.Naming", "CA1721:PropertyNamesShouldNotMatchGetMethods")]
public RepositoryType Type { get; set; }
public RepositoryType? Type { get; set; }

/// <summary>
/// Gets or sets the sort property.
/// </summary>
/// <value>
/// The sort.
/// </value>
public RepositorySort Sort { get; set; }
public RepositorySort? Sort { get; set; }

/// <summary>
/// Gets or sets the sort direction.
/// </summary>
/// <value>
/// The direction.
/// </value>
public SortDirection Direction { get; set; }
public SortDirection? Direction { get; set; }

/// <summary>
/// Gets or sets the visibility property.
/// </summary>
/// <value>
/// The visibility.
/// </value>
public RepositoryVisibility Visibility { get; set; }
public RepositoryVisibility? Visibility { get; set; }

/// <summary>
/// Gets or sets the affiliation property.
/// </summary>
/// <value>
/// The affiliation.
/// </value>
public RepositoryAffiliation Affiliation { get; set; }
public RepositoryAffiliation? Affiliation { get; set; }

internal string DebuggerDisplay
{
Expand Down
3 changes: 3 additions & 0 deletions Octokit/Models/Request/RequestParameters.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ static List<PropertyParameter> GetPropertyParametersForType(Type type)
Justification = "GitHub API depends on lower case strings")]
static Func<PropertyInfo, object, string> GetValueFunc(Type propertyType)
{
// get underlying type if nullable
propertyType = Nullable.GetUnderlyingType(propertyType) ?? propertyType;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😀


if (typeof(IEnumerable<string>).IsAssignableFrom(propertyType))
{
return (prop, value) =>
Expand Down