Fix various system contracts #84
Conversation
There was a problem hiding this comment.
❓ How does this help the contracts? (I never would have thought to include internal methods in the API contracts, but that doesn't mean they aren't useful.)
There was a problem hiding this comment.
It does not help the contracts, but to understand what's going on here any why the contract must be this way.
There was a problem hiding this comment.
💡 Thanks for clarifying. When I was reading through the code in the current form, I didn't feel like I understood exactly why Dispatcher could return null. If the comment for Dispatcher was updated in the way I mentioned in my other comment (on that property), I feel like this internal method and the preceding comment(s) could be removed from the code without reducing the clarity of this set of contracts, as freezing a Freezable is a well-understood concept for API consumers.
There was a problem hiding this comment.
❓ Should this check against 0 as well?
There was a problem hiding this comment.
Right, I've added this.
There was a problem hiding this comment.
❓ Is this missing contracts?
There was a problem hiding this comment.
Yes, I'll add them later.
There was a problem hiding this comment.
❗ I'm fine with that as long as you create an issue. If the issue is narrow-scoped, it might make it easier for a new contributor to come along and add them. An example:
Add contracts for NotifyCollectionChangedEventArgs
A stub contract class for
NotifyCollectionChangedEventArgswas added in #84. The contracts for methods in this class still need to be added.
Then we can mark the issue with the up for grabs label. 👍
There was a problem hiding this comment.
I have added the issue.
However it would be much easier if the one that finds a bug directly creates the issue 😄
|
First of all, this pull request was a massive improvement over #21, in terms of ability to focus the review on changes to contracts. Great job with that, I know it must have been a lot of work. 👍 You've given me a lot to think about. I'll be filing some "interesting issues" (IMO) as a result. Also 👍 for this. I'll read over the answers to the questions later today. |
There was a problem hiding this comment.
❗ This is incorrect. It should be index < Count instead of index <= Count (the documentation above it needs to be corrected as well.
📝 Yes, MSDN is incorrect on this.
|
@SergeyTeplyakov any idea when you will merge this? @sharwell has reviewed it and I don't see any more concerns. |
3 participants
Fix a lot of buggy system contracts