blairconrad / SelfInitializingFakes

Like Fowler's self-initializing fakes.
MIT License
11 stars 3 forks source link

Use json and support tasks #79

Closed CableZa closed 4 years ago

CableZa commented 4 years ago

hi @blairconrad

I'm interested in using the Self initializing fakes project but found that I need to make some changes to support my requirements, the main thing being the need to support Task methods. These cannot be serialized easily using Binary/XML so replaced the recorder with JSON.

I'm not sure how important Binary/XML serialization is to other users of this project? If it is I could look into restoring support for it while still supporting Task.

I also added a TestDoubleUtiltities class to help with some boiler plate code around test recorder setup.

Lastly I upgraded to .net 4.8 and .net core 3.1 as targets.

blairconrad commented 4 years ago

Hi, @CableZa! Thanks for your interest in the project, both as a user and as a contributor. I appreciate your desire to contribute to SelfinitializingFakes, but I am going to have to reject this pull request. This doesn't mean that I'm uninterested in your contribution, just that it should take a different form.

The main reason I'm rejecting the PR is that it just does too much. As far as user-visible changes go, you've

  1. added support for serializing Tasks
  2. added a new built-in serialization format (JSON)
  3. removed the only other 2 built-in serialization formats
  4. added supported platforms
  5. removed support for other platforms that clients are likely using

Each of those would warrant their own pull request, even if I wanted them included in the project. And of the 5 changes, I see potential value in the ones starting with "added", but I'd prefer not to remove the binary and XML serialization formats or drop platform support without serious thought.

In addition to the user-visible changes, some of your other changes would either cause me to reject the PR or ask for significant rework. Based on the sparsity of your GitHub contribution graph, I'm guessing that you've not contributed to many open-source projects before, so I don't expect you to have known about these things in advance. However, I think you'll find that most project owners will have a similar attitude.

I'll list some of the undesirable components below. Remember, this is not a condemnation of you, just an indication that usually OSS project contributions avoid these kinds of changes, at least without negotiation with the project owners beforehand.

  1. several .csproj files are rewritten for what seems no reason, introducing ProjectGuids and expanding <Compile Include directives that had previously had wildcards in them, potentially complicating future maintenance
  2. there's a new TestDoubleUtilities.cs, not connected to the rest of the PR

Here's my plan. Let's split this PR into a few issues that can be discussed. Initially I'll make one for

CableZa commented 4 years ago

Thanks for the detailed feedback @blairconrad , much appreciated!. I and/or a colleague will address the individual issues with PR's.