Skip to content

Conversation

ghostwriter
Copy link
Contributor

  • Mock Filesystem inside TestCase.php
  • Removed MocksFilesystem trait

@jasonmccreary
Copy link
Collaborator

I vaguely remember there being an issue when using Illuminate\Filesystem\Filesystem directly. That's why these used the contract.

It won't be something that appears in the tests since we're mocking, but it will be an issue at runtime. Especially if someone has swapped it out in the container. However, I'm not sure if we'd want to use that anyway. So it's probably fine, but wanted to brain dump before merging to get your thoughts...

@ghostwriter
Copy link
Contributor Author

ghostwriter commented Apr 3, 2021

  • tests were only partially mocking the class.
  • inconsistent usage (concrete/interface)

Blueprint uses both \Illuminate\Contracts\Filesystem\Filesystem and Illuminate\Filesystem\Filesystem interchangeably.

Which I understand as, it accepts the concrete class in some places instead of the interface, and the tests partially mocks the interface.

To your point, i’ll update this PR to use the concrete implementation (I assume since Blueprint is specifically for Laravel, devs wouldn’t need a 3rd party library for filesystem when one is provided “out of the box”.)

@jasonmccreary
Copy link
Collaborator

@nathane, I'm fine either way. To your point, just want it to be consistent.

Copy link
Collaborator

@jasonmccreary jasonmccreary left a comment

Choose a reason for hiding this comment

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

Whatever recent commit you made had very opinionated formatting and importing rules.

@jasonmccreary
Copy link
Collaborator

@nathane, best to undo the "scouting" commit. It has all sort of opinionated formatting and the class reference, while I generally agree with, actually make the code harder to test since it's not within the Laravel framework.

This reverts commit ca99cef.
@ghostwriter ghostwriter requested a review from jasonmccreary May 9, 2021 15:54
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
@jasonmccreary jasonmccreary merged commit d93983d into laravel-shift:master May 10, 2021
@ghostwriter ghostwriter deleted the refactor-filesystem-mock branch May 10, 2021 14:07
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.

2 participants