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. #763

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.

This PR continues #760 which has been split into two smaller PRs.

@ArclightHub ArclightHub force-pushed the allow-config-and-temp-to-be-configured-independently branch from f1a3f0a to 5cfb911 Compare November 27, 2023 06:41
@ArclightHub ArclightHub changed the title Work In Progress - Allow config and temp directories to be configured independently. Allow config and temp directories to be configured independently. Nov 27, 2023
@ArclightHub ArclightHub marked this pull request as ready for review November 27, 2023 07:07
@ArclightHub ArclightHub force-pushed the allow-config-and-temp-to-be-configured-independently branch from 36ffc11 to b653141 Compare November 27, 2023 07:10
@shalvah shalvah merged commit 7263152 into knuckleswtf:master Dec 12, 2023
6 checks passed
@shalvah
Copy link
Contributor

shalvah commented Dec 12, 2023

Sorry for the delay, merged! 🎉

This had the key foundations, I just had to rework some of the naming and internal API choices. Sharing a few notes:

  • A parameter like $isHidden isn't good, because what we want to achieve has nothing to do with hidden/non-hidden, just with automatic inference from the config name. Similarly, I removed some tests focusing on the hidden aspect.
  • A method like getScribeConfigurationPath() is misleading, because, even though it internally uses the configName, one of its purposes is to resolve output paths (for instance, $pathConfig->getScribeConfigurationPath('openapi.yaml')). So I split it into separate methods, so this is now $pathConfig->outputPath('openapi.yaml').
  • Having repeated parameters and booleans like in new PathConfig($configName, $configName, true) is often a suboptimal API, because its unclear to the reader. We can often do better. In this case, I refactored to new PathConfig($configName) (since the others depend on the first, by default). For cases of multiple arguments, we can use PHP 8's named arguments to make it clearer:
    - new PathConfig($configName, $this->option('scribe-dir'));
    + new PathConfig(configName: $configName, scribeDir: $this->option('scribe-dir'))
  • It was a good idea to create a base test method (in the OutputTest), and have a bunch of test methods call it. However, the exposed API was quite noisy. Rather than having $this->base(['--a' => 'aa', '--b' =< 'bb'], 'bb') (unnecessary duplication), this can simply be $this->base('aa', 'bb'), and the base method takes the key parameters and constructs the needed fields itself.

I'm sure this was a big effort. Thanks for the initiative!

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