Skip to content

Adding MockFileData overload to serialize objects - #389

Closed
padillah wants to merge 2 commits into
TestableIO:masterfrom
padillah:padillah-mockfiledata
Closed

padillah wants to merge 2 commits into
TestableIO:masterfrom
padillah:padillah-mockfiledata

Conversation

@padillah

Copy link
Copy Markdown

Added a simple overload to include object serialization to the MockFileData class. I moved the common code to a private method and called it from both the old constructor and the new overload.

@padillah padillah closed this Oct 30, 2018
@padillah padillah reopened this Oct 30, 2018
@fgreinacher

Copy link
Copy Markdown
Contributor

Thanks for your work, but I don't think this is something that needs to be in System.IO.Abstractions for the following reasons:

  • the discoverability of the new overload is low, I wouldn't intuitively know what it does
  • this is a low-level library for IO operations that should stay focused
  • it is easy (and IMO more flexible) to do the serialization on top of S.IO.A

So, I'm sorry, but I'll reject this PR.

For the future I'd suggest you to create an issue that allows discussing the idea before starting a PR.

@fgreinacher fgreinacher closed this Nov 4, 2018
@padillah

padillah commented Nov 4, 2018

Copy link
Copy Markdown
Author

Quick question: If you wanted discussion is there any reason it couldn't be done... right here? In the PR. Where GitHub has provided a space for it?

@fgreinacher

Copy link
Copy Markdown
Contributor

Sure, PR is also fine, but needs more effort on your end (writing the code) and also for the maintainers (reviewing the changes). For new things it’s IMO worthwhile to discuss the idea first (only providing pseudo code if necessary).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants