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

Introduce Classifiers #62

Merged
merged 4 commits into from
Oct 16, 2017
Merged

Introduce Classifiers #62

merged 4 commits into from
Oct 16, 2017

Conversation

jerguslejko
Copy link
Contributor

This PR replaces "ComponentConfiguration" approach for detecting classes discovered in the application.

The core concept is that every class will run through a list of "classifiers". Each classifier performs a set of checks to determine wether it satisfies the given class (ModelClassifier checks wether the given class extends Illuminate\Database\Eloquent\Model etc).

This allows us to isolate checks required to indentify a component. MiddlewareClassifier is a nice example. To classify a middleware, we need to perform multiple checks (check HttpKernel, check local and global middlewares). By using this approach, logic to indentify a middleware is isolated in the MiddlewareClassifier class.

🌵

@stefanzweifel
Copy link
Owner

That's exactly what I had in mind to do. Thanks! 👍

@stefanzweifel
Copy link
Owner

This structure would also make it much easier to disable/enable certain Classifiers in the config file. For example, if you don't care about Migrations you could remove the Classifier from the config

@jerguslejko
Copy link
Contributor Author

With regards to MigrationClassifier as you can see, I have manually marked the test as skipped because I couldn't figure out a way to properly test it.

Well, is it even a good idea to see stats on migrations? It does not seems to be useful getting the number of lines in migrations. Since it does not correlate to the table size, neither is code which is run during the application lifecycle.

I'd suggest removing this functionality.

@stefanzweifel
Copy link
Owner

The LoC count of Migrations and Seeders are not really interesting. I will keep it for now.

Maybe we can hide Migrations and Seeders behind a config option in the future or add a "verbosity" option.

artisan stats -vvv # would show a very detailed report of all components and all found classes

@jerguslejko
Copy link
Contributor Author

I would avoid backing off of the verbosity flag. Something line --include-migrations or -m would me better

@stefanzweifel stefanzweifel merged commit ffbc799 into stefanzweifel:master Oct 16, 2017
@jerguslejko jerguslejko deleted the classifiers branch October 16, 2017 17:52
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