Detect octane availability in composer.json, flavors through binaries… #29
Conversation
|
Need to add test! |
|
|
||
| public function composerJsonContent( $directory ) | ||
| { | ||
| $path = $directory.'/composer.json'; |
There was a problem hiding this comment.
It's not a big difference, but I don't really see why you don't add this in the octaneFlavor function? If you add it there, if you can't find the file you can stop the octaneFlavor function right there
There was a problem hiding this comment.
So some notes on why I separated reading the composer content into it's own function:
- Re-usability
Aside from the octaneFlavor(), the GenerateCommandTest also makes use of the same snippet for reading composer content that I extracted to the function above. In the future, there might be other features that would read the composerjson's content. Soo, we can reuse this function for those as well with a simple call to the composerJsonContent function
- Logic Isolation
If you would notice, the code snippet in octaneFlavor() function only focuses on octane detection. Even though it relies on the composer.json's contents to do it's detection, there's no detailing on how to read the composer file--because it's already determined by calling the composerContentJson() function! I find this neat, as the octaneFlavor() function can just focus on showing its main feature, which is octane+flavor detection, while it can call helper functions like the composerContentJson to do other related, reusable dependencies extraction/calculation/processing.
Note on stopping the detection:
I think it's beneficial that the octaneFlavor() function shows the reason for stopping the detection, which is yknow, after reading that there's no octane in the composer content it retrieved.
app/Services/Scanner.php
Outdated
| */ | ||
| public function octaneFlavor( array $options ) | ||
| { | ||
| // TODO: OCTANE which laravel versions are actually compatible? 11 |
There was a problem hiding this comment.
Should we even check if the laravel and octane versions are compatible? The user has installed them so why would we not follow their setup?
There was a problem hiding this comment.
Yuup you've got a point there! I was deliberating whether it's a necessary evil or not.
Since Octane is only available for later Laravel versions starting from 8, other previous versions might not work with octane at all. Since we already detect the Laravel version of a project, it seemed that including octane detection for those other versions did not make sense. Octane might not even be configured, and in the up chance there's a binary in there, or a laravel/octane in composer, would the Dockerfile created with octane snippets even work, like, will there be dependencies not available in previous Laravel versions required by the Octane setup, causing errors in building the dockerfile when it comes to that point?
What you think?
…rtant in detecting options that rely on existence of file i.e octane flavors; Create test folders for 10 and 11 octane flavors; Include octane-flavor-files(used in detecting the flavors) in the list of files to ignore for matching during test
…iles only when successful, since file generated will help as reference when a test fails
…, include build
What:
Detect whether to configure octane or not! Include flavor detection as well. Set --octane= flag
Why:
Octane usage can be detected through composer.json, and flavor can be detected by flavor binaries included in the project.
SO why not? If it's configured locally in the project. We can include it in the Dockerfile.