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

Adding "FindContainerDependenciesCommand" to the core #45

Closed
FWidm opened this issue Nov 20, 2017 · 5 comments
Closed

Adding "FindContainerDependenciesCommand" to the core #45

FWidm opened this issue Nov 20, 2017 · 5 comments

Comments

@FWidm
Copy link
Contributor

FWidm commented Nov 20, 2017

Hi, I've shown you the FindContainerDependenciesCommand on slack already and you gave me the following feedback:

The only problem is, if the user is injecting the command as dependency and not using the call function. I'd suggest simply adding a warning message when the command is executed to let users know about this.

I've since modified the command to now support both ways to call other scripts - Apiato::call('x@y') and direct uses use App\Contianers\x with two different regex versions. It also parses the container's composer.json to provide more information (you might have two containers with the same name by different authors on different APIs).

My questions:

  1. Was this your mentioned concern with the Apiato::callmethods?
  2. Where would you place the Command & Transformer?

Current version:
Available in the following gist: https://gist.github.com/FWidm/f09aaa852a03e857eabf33990b2fb0af

Sample Output:

$ php artisan apiato:container-dependencies app/Containers/User/
Searching for dependencies in container: app/Containers/User/

 Remove own container from listings? (y/n):
 > y

Found dependencies:
[imports]: 
    [Authorization]: 
        [0]: app/Containers/User/Actions/CreateAdminAction.php
        [1]: app/Containers/User/Data/Seeders/UserPermissionsSeeder_1.php
        [2]: app/Containers/User/UI/API/Transformers/UserTransformer.php

    [Authentication]: 
        [0]: app/Containers/User/Actions/DeleteUserAction.php
        [1]: app/Containers/User/Actions/GetMyProfileAction.php

    [Stripe]: 
        [0]: app/Containers/User/Models/User.php




 Display Container author and description from the composer.json?(y/n):
 > y

[name]: apiato/authorization
[description]: apiato/authorization

[name]: apiato/authentication
[description]: apiato/authentication

[name]: apiato/stripe
[description]: apiato/stripe

@johannesschobel
Copy link
Contributor

Hey there,

Just a few things here:

  1. This is awesome! I really like it! Nice work!
  2. Can we change the signature of your command? I am thinking about something like apiato:dependency:container or whatever? What do you think?
  3. Can you add {} to single-line if-blocks? I think this would make it better readable and easier to extend.
  4. Can you think of any other useful commands that might be required by other users?

@FWidm
Copy link
Contributor Author

FWidm commented Nov 25, 2017

Thanks for the input,
sure, in your discussion post you suggested apiato:list:actions what about renaming it to apiato:list:dependencies {containerName}? It might fit in there as well.

I've updated the gist above to reflect your suggestions, I hope i caught all single-ifs in the snippet, if something catches your eyes, feel free to change it. If you think you've found a good spot for the Command+Transformer you can also push it into the repo, i am still not sure where it'd fit.

@johannesschobel
Copy link
Contributor

Thanks a lot @FWidm ..
Regaring the Command + Transformer thing: Is it possible to have some kind of "inner classes" like in Java? Then, everything related to this command would be in the same class (file). What do you think?

Another way would be to add a /Transformers to the top level and place it there.. What do you think?

Would you provide a PR for us to merge? :)

@FWidm
Copy link
Contributor Author

FWidm commented Nov 27, 2017

Sure, apparently not inner classes are supported in PHP, so I've gone with the second idea of yours. Feel free to move it around if it doesn't fit there.

@johannesschobel
Copy link
Contributor

I'll close this issue for now, because we already have a PR for this ;) Thanks a lot for contributing!

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

No branches or pull requests

2 participants