Skip to content
This repository was archived by the owner on Jul 15, 2023. It is now read-only.

Conversation

@tom-englert
Copy link
Contributor

...DependencyObject.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this maybe throw a NotImplementedException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this would make any difference, CC will only look at the contracts information and ignores any code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm more curious here. I prefer to return as I think it is lighter but was wondering it it made any difference. I suppose it does not matter if we have a standard, not yet at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the stuff is auto-generated, and it would generate a huge effort to change everything, with low benefit.

@aarondandy
Copy link
Contributor

I'm done reading up to 227167b . Some good changes in this PR. Found some stuff I was unsure about but only found one item that I think needs to be addressed: https://github.com/Microsoft/CodeContracts/pull/21/files#diff-cace338ba5980de8d18370a9d6b804e2R389

@aarondandy
Copy link
Contributor

👍

@hubuk
Copy link
Contributor

hubuk commented May 4, 2015

Looks good 👍

@sharwell
Copy link
Contributor

❗ I think this pull request needs to be closed. Changes pushed to tom-englert/master after this pull request was created were automatically appended to this pull request, and I don't think that was intended.

@tom-englert
Copy link
Contributor Author

It was intended for my changes - however everything is already collected together by "hubuk". As long as my changes make it to the main branch, I'm fine with this.

Remove invalid contract from DispatcherObject.
@sharwell
Copy link
Contributor

I partially reviewed this pull request so far. The first two things I note are:

  1. The pull request has definitely been updated at this point to no longer include the changes mentioned in my previous comment.
  2. This pull request is large; the biggest challenge is it contains all of 1) items that need to be merged because they fix errors in the code base (e.g. existing contracts which are flat wrong), 2) items that would be nice to merge because they improve the quality of the code base (e.g. new contracts), 3) items which I am unsure about at this point (e.g. the creation of a new solution file), and 4) items which I would prefer not to merge as part of this pull request (e.g. the large numbers of changes to code formatting which isn't related to the contracts).

Concern was expressed in #62 regarding conflicts in this pull request. I am not very concerned about this right now for the following reasons:

  1. The current conflicts (the reason GitHub doesn't want to merge it) are not a problem for a manual merge of this pull request. @tom-englert would not need to make any changes to this pull request in order for a successful merge to be made.
  2. The ongoing pull requests for build improvements don't overlap or conflict with the primary improvements in this pull request, so it will be relatively easy to prevent new conflicts from arising.

As long as this pull request gets merged before #63 is merged, I believe we are fine.

@tom-englert I've been focused on getting the standard build process updated with the new tooling required for IL rewriting to support the Visual Studio 2015 build tools, and for the editor extensions to support Visual Studio 2013. With the understanding that I really want to get this merged, would you be OK with waiting for me to do a more extensive code review after the build process is updated to account for foundation items like #57, #60, and #66? At that time, if you want I could even prepare new pull requests containing targeted subsets of this code in order to address my concerns about the size of this pull request, without you needing to do any more work on it.

Like I said, this pull request is very important to me. I believe it represents the type of work the majority of Code Contracts contributors will provide over time.

@tom-englert
Copy link
Contributor Author

@sharwell sounds good to me, as long as the changes will make it to the next release I'm fine with everything.

The new solution is just for convenience, it dramatically speeds up development when fixing the existing contracts. It would be helpful to anyone concentrating on contract definitions only, but I won't insist in keeping this in the repo.

I tried my best to not touch the formatting, which is a very hard task with the existing code base without switching of all automatism in all tools. I think there is already an issue requesting to standardize formatting of the existing code, so anyone can work with standard VS settings, which I assume most developers already use. Until this is done I think we have to live with minor formatting and white space changes.

@tom-englert
Copy link
Contributor Author

I have split this into #83 and #84. Both are now based on the latest master and have reduced white space changes, so they can be automatically merged. All reviews from this request should still apply.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants