-
Notifications
You must be signed in to change notification settings - Fork 0
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
Search base class #1
Conversation
FYI this pull request is against your own repo and not the official one. Is that what you intended? |
@johnduhart yep - was just getting some ideas on how to implement this. 😄 I would rather not make one to the octokit.net repo as it may not be important... To be honest - I am not really sure what people do - even if the PR is just an idea and will never get merged, do you still simply make a PR into the main repo? |
@hahmed I do like this as a way to simplify our search request classes. cc @alfhenrik for his thoughts |
Yea, I agree, this will help us keep DRY and will simplify our specific request classes. |
set; | ||
} | ||
|
||
private string SortOrder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SortDirection enum (https://github.com/octokit/octokit.net/blob/master/Octokit/Models/Request/IssueRequest.cs#L91) values has a Parameter(Value="") attribute applied to it so we could create an Enum extensions class and implement this:
public static string ToParameter(this Enum prop)
{
if (prop == null) return null;
var member = prop.GetType().GetMember(prop.ToString()).FirstOrDefault();
if (member == null) return null;
var attribute = member.GetCustomAttributes(typeof(ParameterAttribute), false)
.Cast<ParameterAttribute>()
.FirstOrDefault();
return attribute != null ? attribute.Value : null;
}
Then we can just do Order.ToParameter
and that will convert it to the correct query parameter value.
Although, I'd like @shiftkey's opinion on this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, as we could possible re-use this code elsewhere. As I don't understand reflection too well, I will keep out of this discussion. I kept my code simple so I could understand it easily.
I thought it may be good to make all the searching a bit more consistent by using a base class as we are writing the same code a lot of times.
The file: https://github.com/hahmed/octokit.net/blob/2551ce867e9c3ad3c98256a19566d110a8c6eeb9/Octokit/Models/Request/BaseSearchRequest.cs
I have added an example on how this can be implemented, with the SearchCodeRequest class.
I would love to hear your views @shiftkey and @alfhenrik
octokit#290
(Thanks to both of you guys I learnt a lot from the searching api 👍 )