Changed repos to a specialized collection #842
Conversation
|
What's up with the CI build? |
khellang
force-pushed
the
refine-search-api
branch
from
ffd0796 to
7bd0447
Compare
It's been bumped to MSBuild 14.0 which is triggering a neat PCL/FxCop issue. Reported here |
|
Something like this (just the other way)? http://kristian.hellang.com/building-csharp-6-code-with-appveyor/ |
| return GetEnumerator(); | ||
| } | ||
|
|
||
| public void Add(string owner, string name) |
There was a problem hiding this comment.
Fun fact - you can also have Add(string nameWithOwner) here and both sorts will happily work with the collection initializer syntax.
@haacked should we support both? I still like the succinctness of writing them inline rather than splitting them out...
There was a problem hiding this comment.
I actually had both in there, but decided to delete the overload. I guess it doesn't hurt to have it in, since we still do the same, full, validation 😄
There was a problem hiding this comment.
I added a commit with the single string overloads, you can just strip them out if you decide you don't want both.
|
@khellang nah, I wanted to stick with v12 for the moment. I fought a bunch with the toolchain today, couldn't find a way out. It's a minor thing (I don't need to run FxCop on the PCL build as the code is shared) but an annoyance nonetheless. |
|
I'm not really sure if this builds, because I get all sorts of errors in VS 2015. I had hoped the CI build would tell me, but no luck there either 😛 |
i got you bb. Once @haacked has had a chance to chime in I'll test things on VS2015 proper. |
| @@ -337,4 +336,74 @@ public enum IssueTypeQualifier | |||
| [Parameter(Value = "issue")] | |||
| Issue | |||
| } | |||
|
|
|||
| public class RepositoryCollection : IEnumerable<string> | |||
There was a problem hiding this comment.
Since the repository list will always be a finite collection, should this be an IReadOnlyCollection<string>? And could it not simply inherit a concrete class at that point?
There was a problem hiding this comment.
👍 to more specialized collection
Not sure if the PCL library might get in the way of using a concrete class here though...
There was a problem hiding this comment.
Well there's List that'll work. 😛 Also, it looks like this is supposed to be a mutable collection right now, so maybe List would make more sense than ReadOnlyCollection<T>. Or we could go the immutable route and have Add always return a new collection.
I just think having this implement IEnumerable<T> is not right. At minimum, ICollection<T> would make more sense.
There was a problem hiding this comment.
The only reason it implements IEnumerable<string> is because it's needed for collection initializers to work. I had originally implemented ICollection<string>, but landed on IEnumerable in the original implementation because it didn't allow simple strings.
|
👍 on this. Is our CI having an issue? |
|
@haacked yeah, it's a VS2015 issue (Code Analysis yay) which we got accidentally upgraded to a few days ago details here. I'll address it in #835 which is the base branch for this PR. |
Changed repos to a specialized collection
I didn't remove the validation, since it still gives some value in validating the owner and repo names separately.