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

[RFC]: Next major version of package. Stricter types for methods, API cleaning. #69

Open
lisachenko opened this issue Jan 10, 2021 · 2 comments
Labels

Comments

@lisachenko
Copy link
Contributor

RFC

Q A
Proposed Version(s) 5.x.x
BC Break? Yes

Goal

I want to discuss next major version of this package and what can be improved in it to have better API for this package.

Background

Current version of package contains too many places where method of a class can accept different types at the same time: string/arrays/specific objects. This makes code error-prone and harder to debug, harder to use and increases chances of errors as IDE doesn't help too much when all checks are in runtime. Also static analysis tools complain about types for this library.

Proposal(s)

  1. To add declare(strict_types=1) to all source files to benefit more from stronger type checks
  2. To to add type information to all method signatures (parameter and return types), which will be obviously a BC break, but it will improve code quality a lot.
  3. As 2) will bring a BC break, I want to propose to drop all public static function fromArray(array $array) static constructors from the code, as all these constructors accept non-typed arrays, thus IDE doesn't know about possible keys and can't verify types with static analysis tools. Same should be applied to the AbstractGenerator::setOptions($options) - IMO, it should be removed for the next major version.
  4. Drop all optional arguments from constructors, making ctor signatures smaller and cleaner, eg instead of
    ParameterGenerator::__construct(
        $name = null,
        $type = null,
        $defaultValue = null,
        $position = null,
        $passByReference = false
    );

new signature will be

    ParameterGenerator::__construct(string $name);

As we already have mutation methods in the API, we can use fluent interfaces to adjust generated code item by making required changes:

$parameterGenerator = new ParameterGenerator('test');
$parameterGenerator
    ->setType(TypeGenerator::fromTypeString('string'))
    ->setDefaultValue('foo')
    ->setPassByReference(true);

This should make interface for configuration more stable, as there will be only one way to configure generator via setters that can mutate instance (no more fromArray, no more tons of args in ctors).

  1. Drop all mixed types for method signatures, eg:
    /**
     * Set the default value of the parameter.
     *
     * Certain variables are difficult to express
     *
     * @param  null|bool|string|int|float|array|ValueGenerator $defaultValue
     * @return ParameterGenerator
     */
    public function setDefaultValue($defaultValue);

will be replaced with following signature:

    /**
     * Set the default value of the parameter.
     */
    public function setDefaultValue(ValueGenerator $defaultValue): ParameterGenerator

There are much more examples, eg ClassGenerator::addMethods() that accepts one of 3 possible types of argument.

  1. Close API from inheritance - this library should provide building blocks for code generation, if all main functionality will be covered in the library, then we can close all general classes with final keyword, allowing only composition of code from building blocks.

Considerations

API will change a lot, so definitely we will have a BC break. However, all changes can be reproduced via setters and new API, so migration from current version of code to the new one is just matter of wrapping existing code in external libraries with additional setters and type wrappers.

Appendix

@lisachenko lisachenko added the RFC label Jan 10, 2021
@lisachenko
Copy link
Contributor Author

@Ocramius @weierophinney Could you please check? What is your opinion about this proposal? Thanks in advance!

@Ocramius
Copy link
Member

Overall OK with this: the library does work, but its pain points are the too lax API, coming from a time where "allow everyone to do anything" was the mantra.

Reducing the API surface drastically will help a lot here, and using PHP 8 union types and stricter psalm checks (ideally totallytyped) would help reducing complexity by a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants