Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: introduce convenience methods into MockFileSystem #951

Merged
merged 3 commits into from
Mar 2, 2023

Conversation

rcdailey
Copy link
Contributor

  • AddEmptyFile() added which is a convenience wrapper for AddFile() with an empty MockFileData.
  • AddFile() overload added which consumes an IFileInfo.
  • AddDirectory() overload added which consumes an IDirectoryInfo.
  • GetFile() overload added which consumes an IFileInfo.

Unit test added for AddEmptyFile() but not the others, as the test code for them would have very little value.

Fixes TestableIO/System.IO.Abstractions.Extensions#32

@rcdailey
Copy link
Contributor Author

I realize the PR title workflow is failing, however I would like the project maintainers to introduce whatever prefix they deem appropriate when they squash merge my PR into the mainline. Thank you.

@vbreuss
Copy link
Member

vbreuss commented Feb 26, 2023

Just a quick question from my side:
In TestableIO/System.IO.Abstractions.Extensions#32 you discussed adding it as extension methods to a separate library or adding it directly to the MockFileSystem.

Why not add it as extension methods in this repository?

@rcdailey
Copy link
Contributor Author

Just a quick question from my side: In TestableIO/System.IO.Abstractions.Extensions#32 you discussed adding it as extension methods to a separate library or adding it directly to the MockFileSystem.

Why not add it as extension methods in this repository?

I didn't see a reason to add them as extension methods. Typically I only do that when I do not have access to the class in question. However, in my mind at least, the entire point of contributing these upstream (into this repo) was to make them a part of the class. I already have these as extension methods in my own code base and they were working fine.

I'm unfamiliar with any design policies or preferences in this repo or of its its maintainers, so I went about this using my own judgement. Is there a problem with having them in the class itself?

@vbreuss
Copy link
Member

vbreuss commented Feb 26, 2023

Just a quick question from my side: In TestableIO/System.IO.Abstractions.Extensions#32 you discussed adding it as extension methods to a separate library or adding it directly to the MockFileSystem.
Why not add it as extension methods in this repository?

I didn't see a reason to add them as extension methods. Typically I only do that when I do not have access to the class in question. However, in my mind at least, the entire point of contributing these upstream (into this repo) was to make them a part of the class. I already have these as extension methods in my own code base and they were working fine.

I'm unfamiliar with any design policies or preferences in this repo or of its its maintainers, so I went about this using my own judgement. Is there a problem with having them in the class itself?

@rcdailey :
Please don't get me wrong. Your solution is fine and I think that the convenience methods are a nice addition here!

I am just curious, if there is a specific reason why you implemented it this way, as you had to adapt the IMockFileDataAccessor interface and add quite a lot of methods there. Normally I try to keep interface definitions as minimal as possible.
By adding your implementation as extension methods in this repository, the interface could remain smaller.

Are there disadvantages to extension methods that I am not aware of :-)

@rcdailey
Copy link
Contributor Author

rcdailey commented Feb 26, 2023

I added them to the interface because everything else seemed to have been added there. I honestly didn't think too hard about it. Should I remove them from there? For my own understanding, how do you decide between adding to the interface or not? Happy to learn & adjust. I am not too strongly opinionated on how I went about this. I'm relying on you fine folks to let me know how it should be done :-)

Thanks for the discussion!

@rcdailey
Copy link
Contributor Author

rcdailey commented Feb 26, 2023

For what it's worth, I have these already in my own project as a set of extension methods. I'm happy to drop them in here just like this if that's what you want. Code is here.

I think the reason I jumped right into making them part of the class was @fgreinacher's comment on the linked issue:

I'm also fine adding them to MockFileSystem.

Maybe I read that too literally. It's honestly not a big deal either way. Let me know how you'd like it done.

@fgreinacher
Copy link
Contributor

I added them to the interface because everything else seemed to have been added there.

I'd also tend to add them to MockFileSystem directly for consistency. I don't see a lot of value introducing another class for these methods. But I might very well be missing something :)

@rcdailey
Copy link
Contributor Author

I've force-pushed the requested changes and I've rebased onto latest changes in main.

- `AddEmptyFile()` added which is a convenience wrapper for `AddFile()`
  with an empty `MockFileData`.
- `AddFile()` overload added which consumes an `IFileInfo`.
- `AddDirectory()` overload added which consumes an `IDirectoryInfo`.
- `GetFile()` overload added which consumes an `IFileInfo`.

Unit test added for `AddEmptyFile()` but not the others, as the test
code for them would have very little value.

Fixes TestableIO/System.IO.Abstractions.Extensions#32
@fgreinacher
Copy link
Contributor

fgreinacher commented Feb 27, 2023

Looks good to me now, thanks for your work!

CI seems to be failing due to a .NET bug. We have to wait a bit until the fix is rolled out: dotnet/core#8194 (comment)

@fgreinacher fgreinacher changed the title Introduce convenience methods into MockFileSystem feat: introduce convenience methods into MockFileSystem Mar 2, 2023
@fgreinacher fgreinacher enabled auto-merge (squash) March 2, 2023 19:56
@fgreinacher fgreinacher merged commit 04e117c into TestableIO:main Mar 2, 2023
vbreuss pushed a commit to Testably/Testably.Abstractions that referenced this pull request Mar 3, 2023
…19.2.1 (#269)

[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[TestableIO.System.IO.Abstractions](https://togithub.com/TestableIO/System.IO.Abstractions)
| nuget | minor | `19.1.18` -> `19.2.1` |

---

### Release Notes

<details>
<summary>TestableIO/System.IO.Abstractions</summary>

###
[`v19.2.1`](https://togithub.com/TestableIO/System.IO.Abstractions/releases/tag/v19.2.1)

#### What's Changed

- chore(deps): update dependency dotnet-sdk to v7.0.200 by
[@&#8203;renovate](https://togithub.com/renovate) in
[TestableIO/System.IO.Abstractions#947
- chore(deps): update dependency benchmarkdotnet to v0.13.5 by
[@&#8203;renovate](https://togithub.com/renovate) in
[TestableIO/System.IO.Abstractions#948
- chore(deps): update dependency microsoft.net.test.sdk to v17.5.0 by
[@&#8203;renovate](https://togithub.com/renovate) in
[TestableIO/System.IO.Abstractions#949
- chore(deps): update dependency nunit3testadapter to v4.4.0 by
[@&#8203;renovate](https://togithub.com/renovate) in
[TestableIO/System.IO.Abstractions#952
- chore(deps): update danielpalme/reportgenerator-github-action action
to v5.1.18 by [@&#8203;renovate](https://togithub.com/renovate) in
[TestableIO/System.IO.Abstractions#953
- chore(deps): update dependency snapshooter.nunit to v0.13.0 by
[@&#8203;renovate](https://togithub.com/renovate) in
[TestableIO/System.IO.Abstractions#954
- chore(deps): update dependency nunit3testadapter to v4.4.2 by
[@&#8203;renovate](https://togithub.com/renovate) in
[TestableIO/System.IO.Abstractions#955
- feat: introduce convenience methods into MockFileSystem by
[@&#8203;rcdailey](https://togithub.com/rcdailey) in
[TestableIO/System.IO.Abstractions#951

**Full Changelog**:
TestableIO/System.IO.Abstractions@v19.1.18...v19.2.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/Testably/Testably.Abstractions).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNTMuMiIsInVwZGF0ZWRJblZlciI6IjM0LjE1NC41In0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@github-actions
Copy link

This is addressed in release v19.2.1.

@github-actions github-actions bot added the state: released Issues that are released label Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: released Issues that are released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MockFileSystem extensions
3 participants