Skip to content

Conversation

devmsh
Copy link
Contributor

@devmsh devmsh commented Jul 30, 2020

Sorry for suggesting too many conflicting ideas. 😕

When I start thinking about the big picture and think how must we test the trace command itself, I realized that the command includes the full implementation in contrast to the build command which depends on Buillder class.

The PR is not ready for merging yet, but I want to make sure that you are ok with the approach of separating the command from the logic itself. If yes, this will make it easier to call the Tracer in other command and test if it's called or not without calling a command from another command?

I really enjoyed reading and refactoring the code, and I think I'm very close to start contributing new features and community requested enhancements.

@devmsh devmsh changed the title [WIP] lighter trace command to increase the reusability and testability lighter trace command to increase the reusability and testability Aug 15, 2020
@devmsh
Copy link
Contributor Author

devmsh commented Aug 15, 2020

@jasonmccreary the PR is ready for review.

On the live session, you modify it_calls_the_trace_command but the new test did not really test that, it become a false positive test.

I modify the test to relay on the new Tracer class, and add the command tests for the trace command itself.

Waiting for your comment before submitting the next PR.

@jasonmccreary
Copy link
Collaborator

Yes, I am aware of that. Was waiting on this PR to finalize the direction. 👍

@jasonmccreary jasonmccreary merged commit 7066ef4 into laravel-shift:master Aug 17, 2020
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