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

Allow config and temp directories to be configured independently. #760

Conversation

ArclightHub
Copy link
Contributor

Whats changed?

  • Added the ability to configure the scribe.php (or custom php config file) independently of the temp directory.
  • Added a docker-compose setup to allow others to quickly run the test suite on their local machines without needing to install the test stack onto their environment.

How do I run the tests in the new docker container?

  • Simply run clear && docker-compose up -d && docker logs -f scribe_app_1 and the tests are run and results printed into your terminal!

@@ -23,9 +24,9 @@ class ApiDetails

private array $lastKnownFileContentHashes = [];

public function __construct(DocumentationConfig $config = null, bool $preserveUserChanges = true, string $docsName = 'scribe')
public function __construct(DocumentationConfig $config = null, bool $preserveUserChanges = true, CacheConfiguration $docsName)
Copy link
Contributor

@shalvah shalvah Nov 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix this? (The other occurrence(s) too).

Copy link
Contributor

@shalvah shalvah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the goal here, but I think the API can be improved. I thnk there are quite a few naming mismatches, which make it harder to follow ($configObject implies this is the general Scribe config, but it's a very limited subset of the configuration; scribeConfig implies a config object, but it's a file, etc). Additionally, lines like new CacheConfiguration($configName, $configName, true) are not very expressive.

@@ -24,12 +28,12 @@ public static function fromApp(
GenerateDocumentation $command,
RouteMatcherInterface $routeMatcher,
bool $preserveUserChanges,
string $docsName = 'scribe'
CacheConfiguration $docsName
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like mismatching parameter names? Name $docsName and type CacheConfiguration (also in the other places in this file)

Storage::disk('local')->put("{$this->docsName}/collection.json", $collection);
$collectionPath = Storage::disk('local')->path("$this->docsName/collection.json");
Storage::disk('local')->put($this->docsName->getScribeConfigFile() . "/collection.json", $collection);
$collectionPath = Storage::disk('local')->path($this->docsName->getScribeConfigFile() . "/collection.json");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're doing this, perhaps we should move away from concatenation, and just make it a resolver (a la Laravel's storage_path(...))? Somethig like:

Storage::disk('local')->path($this->paths->outputFile("/collection.json"));

@ArclightHub
Copy link
Contributor Author

I will split this into two separate PRs

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