Skip to content

Refactor TypeGenerator #845

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

Merged

Conversation

murtukov
Copy link
Contributor

@murtukov murtukov commented May 9, 2021

Q A
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Documented? yes
Fixed tickets #843

This is a draft.

@mcg-web I think the GraphQL configs ($types argument) should be moved away from TypeGeneratorOptions because it's not an option, but actual data the generator works with.

I also kept the constructor in the TypeGeneratorOption cause it ensures the object to be properly initialized. Without a constructor the object can have uninitialized properties, which is not desired:

$options = new TypeGeneratorOptions();
$options->cacheDir = "...";

$myVal = $options->cacheDirMask; // throws an error, property uninitialized

The getCacheDir method was simplified and renamed into getCacheDirOrDefault. The argument is not needed anymore, because if you wanna access the cacheDir without the fallback to default you can simply access it directly: $options->cacheDir

@murtukov murtukov linked an issue May 9, 2021 that may be closed by this pull request
* Move generator options into a separate class
* Update UPGRADE.md
@murtukov murtukov force-pushed the refactoring/decouple_type_generator_props branch from 18b58e0 to 1381982 Compare May 9, 2021 15:57
@murtukov
Copy link
Contributor Author

murtukov commented May 10, 2021

@mcg-web ok, so now the options cannot be accessed outside of TypeGenerator class, except the ones that are exposed via methods:

  • TypeGenerator::getCacheDir()
  • TypeGenerator::getCacheDirMask()
  • TypeGenerator::getCacheBaseDir()
  • TypeGenerator::setCacheBaseDir()

What about this?

I think the GraphQL configs ($types argument) should be moved away from TypeGeneratorOptions because it's not an option, but actual data the generator works with.

@mcg-web
Copy link
Contributor

mcg-web commented May 10, 2021

I think the GraphQL configs ($types argument) should be moved away from TypeGeneratorOptions because it's not an option, but actual data the generator works with.

We can indeed inject $types directly in TypeGenerator.

@murtukov murtukov marked this pull request as ready for review May 10, 2021 22:32
@murtukov murtukov merged commit 3af1a4a into overblog:0.14 May 11, 2021
@murtukov murtukov deleted the refactoring/decouple_type_generator_props branch May 11, 2021 08:05
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.

Simplify TypeGenerator dependencies
2 participants