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

Separate the concern of getting groupedEndpoints by creating different strategies #263

Merged
merged 2 commits into from
Jul 5, 2021

Conversation

ajcastro
Copy link
Contributor

@ajcastro ajcastro commented Jul 2, 2021

Separate the concern of getting groupedEndpoints by creating different strategies for getting groupedEndpoints.

Before, the command class GenerateDocumentation contains all the helper methods in getting the groupedEndpoints.
In this PR, I refactor it by creating different strategies in getting the groupedEndpoints.

When I read the code, currently there are two ways of getting the groupedEndpoints:

  1. First is extracting the endpoints from the Laravel app, I call this strategy class GroupedEndpointsFromApp
  2. Then the other one is getting it from the generated YAML from the camelDir, I call this GroupedEndpointsFromCamelDir.

So first there is GroupedEndpointsFactory that will return the appropriate strategy, either GroupedEndpointsFromApp if it isForcing or shouldExtract is true. Otherwise, it will use GroupedEndpointsFromCamelDir.

This PR is important because later on we can add another strategy on getting the groupedEndpoints which is I plan to do next,
GroupedEndpointsFromTests, which returns the groupedEndpoints from the performed phpunit tests which I mention in TDD or Test-Driven Documentation, #260

This PR seems to have a lot of file changes but basically what I just did here is just moved the helper methods together for the GroupedEndpointsFromApp. Then I created another strategy for GroupedEndpointsFromCamelDir and create GroupedEndpointsFactory

@shalvah
Copy link
Contributor

shalvah commented Jul 2, 2021

Well, that was fast.

@shalvah
Copy link
Contributor

shalvah commented Jul 2, 2021

Hmmm, makes sense. I'll need to do a bit of testing though. Sadly there aren't enough (or any) automated tests around this part of the code yet.

@shalvah shalvah self-assigned this Jul 2, 2021
@ajcastro
Copy link
Contributor Author

ajcastro commented Jul 2, 2021

@shalvah Sure. Thanks! I'm excited about this. 😄

@ajcastro
Copy link
Contributor Author

ajcastro commented Jul 3, 2021

@shalvah I created another refactoring and started some POC, ajcastro#1

@shalvah shalvah merged commit 30e4850 into knuckleswtf:master Jul 5, 2021
@shalvah
Copy link
Contributor

shalvah commented Jul 5, 2021

I've added some more tests, so I'm taking a look at this now, and it's good.👍

However, I'm not so sure about the full-fledged tests approach yet. Two major reasons:

  1. I'm not sure if I'm ready to maintain this. In fact, considering my plans and goals right now, I don't think I can. (And yes, you could say you would, but I won't be able to not worry about what's in my codebase and how everything interacts.)
  2. I haven't decided how it fits into Scribe For example, I need to decide if this is the primary approach we'll point people to. Multiple places in the docs may need to be updated to cater for this, and I need to decide how. And then there'll be people that will have issues and questions. I'm not yet convinced of the tests strategy, so I won't be very helpful there.

On the other hand, thanks to the whole Camel thing + the factory approach you just added, it's easier to do this in userland. Since the GroupedEndpoints factory is injected into the handle() method, you can bind a new factory in the DI container that includes the tests strategy instead, as long as output is in the Camel format. Hell, you could probably just pull in most of the code from the other package and convert the output into Camel objects.

So here's what I recommend—let's make it a plugin. You can get this to work and publish it, and in the docs, I'll point people to it if they prefer that approach. Over time, if it does well, and I take a liking to it, we can then bundle it with Scribe out of the box. 👍

@ajcastro
Copy link
Contributor Author

ajcastro commented Jul 6, 2021

Thanks @shalvah . I like the idea of making it a plugin. I'll let you know if it is ready, currently, I am still working on it.

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