Adds support for alternate accept header to retrieve star creation timestamps #1060
Conversation
|
Sorry, CI is coming up with errors I didn't catch on my local machine. Almost got it hammered out. I'll squash when I'm done and let you know when this is good for review. |
|
Okay, this should be good to go now. Travis is still reporting an error - looks like something in Octokit.Tests-Portable, but I don't know how to make heads or tails of it. Just let me know if it's something else I need to clean up. |
|
@daveaglick all good, there's some CI friction that I need to look into at some stage |
| [DebuggerDisplay("{DebuggerDisplay,nq}")] | ||
| public class RepositoryStar | ||
| { | ||
| public RepositoryStar() { } |
There was a problem hiding this comment.
You mean the empty ctors or the whole wrapping classes?
There was a problem hiding this comment.
The empty ctors. I ask because we've gone back and forth on how the response models should be structured in the past - previously the properties were public set-ers, but then we switched over to protected set to indicate which were required.
cc @haacked for feels
There was a problem hiding this comment.
The main benefit for using protected set was around testability, but I'm not sure that's so important now in hindsight...
There was a problem hiding this comment.
Ah, gotcha. I just assumed they were needed by the model deserializer. If it can instantiate models without the empty public ctor, I'm fine removing them.
There was a problem hiding this comment.
Ugh, that's a good point - I had a look at some other response models and found they also have an empty ctor - hold off on making that change, I'll let @haacked refresh my memory here...
There was a problem hiding this comment.
If memory serves me, we do need the default ctor for serialization.
|
@daveaglick thanks for this, and apologies for not getting to it weeks ago 😁 |
Adds support for alternate accept header to retrieve star creation timestamps
|
👍 Thanks, no problem! |
Resolves #1059