[WIP] Move to version 1.0 #68

cristianoc72 · 2021-04-10T17:08:11Z

Current Status

Changes

Bump PHP to version 8+ and update all dependencies.
Refactor the parser to infer the types firstly from the code and then from docblocks
Remove php-code-profiles and php-code-formatter
Merge CodeFileGeneratorConfig into CodeGeneratorConfig
'declareStrictTypes' now is true by default
Introduce Twig template engine and move all the render logic into some Twig templates and one extension (see below)
Now the library fully supports phootwork (default) and PSR-12 code styles. We introduce a new configuration property codeStyle to choose between them.

Builders refactor

With this PR, gossi\codegen\generator\CodeGenerator class is the only responsible of the model generation: all the building and writing logic happens into Twig templates and one extension gossi\codegen\utils\TwigExtension.
Now, it's possible to create our own code style by simply copy-paste-modify one or more templates.

You can find the templates into resources/templates directory.
Into resources/templates/default there are the files to render the code with the default (phootwork) code style, while into resources/templates/psr-12 there are the psr-12 templates.
All the files have self-explained names (i.e. class.twig contains the logic to render the PhpClass objects) so it's very easy to understand which template to overwrite.
Let's show it by an example.

Let's suppose we like PSR-12 code style but we want the declare(strict_types=1) instruction on the same line, one space after <?php.

Firstly, we need to create a directory, (i.e. my/project/templates/dir) into our project, to put the modified file.
CodeFileGenerator object writes the file by resources/templates/psr-12/file.twig so let's copy-paste it into my/project/templates/dir.
let's open my/project/templates/dir/file.twig, remove the first three lines and write:

<?php{% if config.declareStrictTypes %} declare(strict_types=1);{% endif %}

Now, we have to configure our CodeFileGenerator to use the psr-12 code style and to search the templates in our own directory firstly:

<?php

$generator = new CodeFileGenerator([
	'codeStyle' => 'psr-12',
	'templatesDirs' => ['my/project/templates/dir']
]);

That's all! Now, the generator searches the needed templates firstly into our project directory, then into resources/templates/psr-12.

Note: if the configured templates directory doesn't exists, the genrator constructor rises an exception.

- PHP8+ strictly typed - Bump dependencies to the latest version - Move render logic from builders to some Twig templates and one Twig extension - Refactor the parser to infer types both from code and docblock

Refine generator and fix the generation of method body. Add more tests. Refactor parser to infer the types from the code first, than from the docblock.

Fix all Psalm errors, up to level 5.

gossi

Thanks for the huge effort you put into this. I started reviewing but had to pause (I put a comment on how far I went) and I left comments until there.

As I'm not fully through yet, my question is: What's the advantage of using twig over writer + code-profiles + code-formatter (especially as the latter two are external to this and can be developed independently)?

gossi · 2021-04-11T09:27:23Z

CompleteClass.php

@@ -0,0 +1,42 @@
+<?php declare(strict_types=1);


What's the purpose of this file? A fixture?

Why is that file in repo root?

Error! I forgot to remove it. It's a fixture.

gossi · 2021-04-11T09:35:28Z

src/generator/CodeFileGenerator.php

@@ -16,62 +21,12 @@
 * @author Thomas Gossmann
 */
 class CodeFileGenerator extends CodeGenerator {


seems like, this file is kept for backwards compatibility reasons? Then mark it @deprecated

gossi · 2021-04-11T09:42:08Z

src/generator/CodeGenerator.php

+		return $this->twig->render($template, ['model' => $model, 'config' => $this->config]);
+	}
+
+	protected function sortModel(GenerateableInterface $model): void {


this method itches me. Before with the builders, each respective builder was responsible for its own model (and what it was sorting). Now everything in one class couples this class to every, this is not very robust in terms of architecture and later maintanability as borders become blurry. Better have a SOLID arch here (see comment above).

gossi · 2021-04-11T09:45:04Z

src/generator/CodeGenerator.php

 	 */
 	public function generate(GenerateableInterface $model): string {
-		return $this->generator->generate($model);
+		$this->sortModel($model);


To have clear responsibilites, what about:

$modelGenerator = ModelGeneratorFactory::create($model, $this->config); $modelGenerator->sort();

it keeps the idea of ModelBuilder but extracts the building part of it. Keeping it with a class that has responsibilities for each model type and what to do with each model as prep for turning code into their respective strings.

Maybe understand it as these ModelGenerators do the meta work before generation and the CodeGenerator (as a generic class) orchestrates the ModelGenerator and connects it to the writer (twig now).

PS. Not necessarily name it ModelGenerator (it was just the name been used before). I think there is a better name then.

gossi · 2021-04-11T09:51:25Z

src/generator/comparator/DefaultConstantComparator.php

 	 */
-	public function compare($a, $b) {
+	public function compare(mixed $a, mixed $b): int {


Ah shooot. I was suggesting this:

Suggested change

public function compare(mixed $a, mixed $b): int {

public function compare(PhpConstant $a, PhpConstant $b): int {

until I realized without mixed it wouldn't match the Comparator interface. Dang php, it's time for generics 😂

gossi · 2021-04-11T10:02:22Z

src/model/AbstractPhpStruct.php

 	 * @return $this
 	 */
-	public function setRequiredFiles(array $files) {
-		$this->requiredFiles = new Set($files);
+	public function setRequiredFiles(array|Set $files): self {


what about?

Suggested change

public function setRequiredFiles(array|Set $files): self {

public function setRequiredFiles(iterable $files): self {

I checked, that Set uses Iterator - dunno exactly which is the more broader typehint here (would also upstream this to phootwork then).

gossi · 2021-04-11T10:05:05Z

src/model/AbstractPhpStruct.php

 	 * @return bool
 	 */
 	public function hasUseStatement(string $qualifiedName): bool {
 		return $this->useStatements->contains($qualifiedName);
 	}

+	public function hasUseStatementsToRender(): bool {


Suggested change

public function hasUseStatementsToRender(): bool {

public function needsUseStatementGeneration(): bool {

render seems a bit out of context, we want to know whether to generator them or not.

gossi · 2021-04-11T10:07:48Z

src/model/ConstantsInterface.php

@@ -1,8 +1,17 @@
-<?php
-declare(strict_types=1);
+<?php declare(strict_types=1);


// --- I only got so far ---

cristianoc72 · 2021-04-11T17:22:35Z

As I'm not fully through yet, my question is: What's the advantage of using twig over writer + code-profiles + code-formatter (especially as the latter two are external to this and can be developed independently)?

Well, my idea was to replace code-profiles and code-formatter libraries (which are still in early state) with something else easy to maintain (for us) and easy to customize for the users.
In particular, imho this library is blocked by php-code-formatter, which needs a lot of love. I tried to study the code but I think I'm not skilled enough for it: after all, I'm just a poor amateur! 😄

Anyway, I've submitted a work-in-progres PR just to discuss this details: it's a very big change and, maybe, from the architectural point of view, the original code is better.

During the next week, I'll try to study php-code-formatter again 😅

LTSCommerce · 2021-07-07T13:16:15Z

hi guys, thanks for all the efforts on this PR

I wondered if this is still happening and has a good chance of making it into master?

cristianoc72 · 2021-07-07T15:16:13Z

@LTSCommerce I'm sorry I left this pr pending, I've been busy till now. Anyway, it's time to really move forward this awesome library.

@gossi please, show us the way! 😄 If you like this refactor, I'll work on it to include all your suggestions, if you prefer the profiler-formatter approach, I'll try to move forward gossi/php-code-formatter library first.
Don't worry: I know that you are kind and nice (and a true genius, too!): I do not take offense if you tell me that this refactor makes you sick! 😆

LTSCommerce · 2021-07-07T15:34:57Z

Great thanks for replying @cristianoc72

For me, I would just love to get this library working with the latest PHP Parser and PHP8 in general which it looks like this PR has mostly achieved. Note I am seeing some bugs, could open a PR against your fork if you want @cristianoc72 ... getting a bit complex though

first one I found is \gossi\codegen\parser\visitor\parts\ValueParserPart::getExpression

The match statement does not match all possible nodes, so I added a meaningful exception and also added the one that I found so far:

	private function getExpression(Node $node): mixed {
		return match (true) {
			$node instanceof ConstFetch => $this->constMap[$node->name->parts[0]] ?? $node->name->parts[0],
			$node instanceof ClassConstFetch => (!$node->class instanceof Node\Name ?: $node->class->parts[0]) . '::' . $node->name,
			$node instanceof MagicConst => $node->getName(),
			$node instanceof Array_ => (new PrettyPrinter())->prettyPrintExpr($node),
			$node instanceof BinaryOp => $node->getOperatorSigil(),
			default => throw new \RuntimeException('Unexpected '.$node::class . ' in '. __METHOD__)
		};

gossi · 2021-07-08T14:33:39Z

This is how far I got ... after ~3 hours of review:

I wanted to come back, but never did - and even now, I would probably need to start from the top again for context (not detailed view). This is just sharing my current status.

I'm not holding against it. I find it super nice, this is still in use and people are here to maintain it and even drive it forward - this is amazing.

So, how we best make this possible? To be honest, this is too much to review in one go. An idea is to chunk this down into maintainable and reviewable ones. I have an idea: Make a new branch from master: 1.0. Split this PR apart into "meaningful units". First one could be to only change the low-level models. Second the buildings blocks onto the new models and next one the layer consuming these, etc. I think the tests will go red all the way until the (we already see, this is coming together green) but this is perfectly fine to me.

Also thanks to @LTSCommerce for jumping in. Would you provide a helping hand for the review?

Would that work?

LTSCommerce · 2021-07-08T14:43:45Z

Happy to help if I can

For now I've made a fork of the @cristianoc72 fork so that I can continue to work on updating the package I'm working on

I've no deep experience with this code base beyond just being a user, but I do definitely approve of using Twig for the code templating, it's a good idea.

For me the priority is something that works and I can use and so far with this version it seems to be doing that pretty much though I'm still testing.

cristianoc72 · 2021-07-08T17:42:53Z

@gossi Great! I'll start soon the PR-splitting and, while doing it, I'll include all yours and @LTSCommerce's suggestions! When I'm ready with the first chunk, I'll close this PR and submit a new one against 1.0 branch.

Thank you!

LTSCommerce · 2021-07-08T19:17:19Z

Awesome

cristianoc72 · 2021-07-09T05:55:51Z

Ready for the first chunk. @gossi please, could you create the 1.0-dev branch? Thank you!

gossi · 2021-07-10T11:14:44Z

done. Can't you do that on your own? If not I need to check permissions

cristianoc72 · 2021-07-10T12:36:27Z

No,I can't: not enough permissions for this repo.

gossi · 2021-07-10T13:59:32Z

Ah, I checked. For repos on personal accounts, there is only collaborator and repo owners and they have fixed access rights. I thought you were able to do this on your own, my bad. So the personal repo actually doesn't really work for this then. I think, there are some repos where you are actually maintaining them rather then me. I see them as:

php-code-generator
docblock
php-code-formatter *
php-code-profiles *
swagger *

* basically not all of them, but they belong to the collection of my open php repositories (and somehow they depend on each other).

As I've learned, they are better worked on, on an orga account, than my personal one. I would set them free to move into an org and be assisting there. Either phootwork or another one? Would you be up for it? WDYT?

cristianoc72 · 2021-07-12T05:41:05Z

Yes, moving the libraries to an organization is a good idea, for sure!
We can move them to phootwork: after all, it's a collection of libraries, isn't it? We could re-arrange it's documentation and api doc to include the newcomers libraries.

Anyway, if you like to leave phootwork as is, because it's a coherent and easily understandable repo, creating a new organization it's ok for me. Please choose the name, you are better than me! The only name that came to my mind is gossilibs 😆

cristianoc72 · 2021-07-14T05:43:37Z

PR #69 opened. I'll close this one, when we decide the organization for this repo.

gossi · 2021-07-14T08:03:24Z

phootwork to me is really a low-level thing. All the other packages I mentioned are of higher level (they all? use phootwork though). Would go for a different org then. I see it similar to php league or so.
I'm off for vacation until end of july. Maybe coming back with a name afterwards? Please feel free to think of one yourself :)

cristianoc72 · 2021-08-05T15:14:50Z

@gossi continuing on the road of breakdance, the name could be phpowermove 😄

gossi · 2021-08-05T16:25:56Z

oh yes, please. This is brilliant !!! Please create the org, so I can start transfer over the repos

gossi · 2021-08-05T16:27:07Z

I had no idea. And this morning I checked if there is a disney princess with f in her name to replace with ph (in german this is almost the same sound) .... but no and a couple of hours later you came around with this idea. Truly awesome!

LTSCommerce · 2021-08-05T17:07:41Z

Feeling privileged to witness the birth of something new, a piece of history... 👍

cristianoc72 · 2021-08-06T05:15:02Z

I had no idea. And this morning I checked if there is a disney princess with f in her name to replace with ph (in german this is almost the same sound) .... but no and a couple of hours later you came around with this idea. Truly awesome!

😄 I'm glad you like it!
The organization is on the road!

gossi · 2021-08-11T12:48:39Z

I moved all my projects to the new org. Sooo happy with the name.

I'll let it up to you to figure out how to best send PRs for this new repo.

Shall we close this one?

gossi · 2021-08-11T12:56:34Z

oooh, maybe you can create a team for this org. Afair within such a team, we have a chat. So we can discuss how to organize things in this org.

Dunno if helpful but at least I'd give it a try.

cristianoc72 · 2021-08-11T14:26:58Z

Great!!
I'm gonna close this PR.
Firstly, I'll submit a PR to change the namespace for the classes, then I'll rebase #69 and continue from there.

cristianoc72 added 4 commits April 7, 2021 07:40

Move forward

625574e

- PHP8+ strictly typed - Bump dependencies to the latest version - Move render logic from builders to some Twig templates and one Twig extension - Refactor the parser to infer types both from code and docblock

Generator and parser

6f41121

Refine generator and fix the generation of method body. Add more tests. Refactor parser to infer the types from the code first, than from the docblock.

Introduce PSR-12 templates

9fa1583

Psalm level 5

a908dae

Fix all Psalm errors, up to level 5.

gossi reviewed Apr 11, 2021

View reviewed changes

cristianoc72 closed this Aug 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Move to version 1.0 #68

[WIP] Move to version 1.0 #68

cristianoc72 commented Apr 10, 2021

gossi left a comment

gossi Apr 11, 2021

cristianoc72 Apr 11, 2021

gossi Apr 11, 2021

gossi Apr 11, 2021

gossi Apr 11, 2021

gossi Apr 11, 2021

gossi Apr 11, 2021

gossi Apr 11, 2021

gossi Apr 11, 2021

cristianoc72 commented Apr 11, 2021

LTSCommerce commented Jul 7, 2021

cristianoc72 commented Jul 7, 2021

LTSCommerce commented Jul 7, 2021

gossi commented Jul 8, 2021

LTSCommerce commented Jul 8, 2021

cristianoc72 commented Jul 8, 2021

LTSCommerce commented Jul 8, 2021

cristianoc72 commented Jul 9, 2021

gossi commented Jul 10, 2021

cristianoc72 commented Jul 10, 2021 •

edited

gossi commented Jul 10, 2021

cristianoc72 commented Jul 12, 2021

cristianoc72 commented Jul 14, 2021

gossi commented Jul 14, 2021

cristianoc72 commented Aug 5, 2021

gossi commented Aug 5, 2021

gossi commented Aug 5, 2021

LTSCommerce commented Aug 5, 2021

cristianoc72 commented Aug 6, 2021

gossi commented Aug 11, 2021

gossi commented Aug 11, 2021

cristianoc72 commented Aug 11, 2021

	public function compare(mixed $a, mixed $b): int {
	public function compare(PhpConstant $a, PhpConstant $b): int {

	public function setRequiredFiles(array\|Set $files): self {
	public function setRequiredFiles(iterable $files): self {

	public function hasUseStatementsToRender(): bool {
	public function needsUseStatementGeneration(): bool {

[WIP] Move to version 1.0 #68

[WIP] Move to version 1.0 #68

Conversation

cristianoc72 commented Apr 10, 2021

Current Status

Changes

Builders refactor

gossi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc72 commented Apr 11, 2021

LTSCommerce commented Jul 7, 2021

cristianoc72 commented Jul 7, 2021

LTSCommerce commented Jul 7, 2021

gossi commented Jul 8, 2021

LTSCommerce commented Jul 8, 2021

cristianoc72 commented Jul 8, 2021

LTSCommerce commented Jul 8, 2021

cristianoc72 commented Jul 9, 2021

gossi commented Jul 10, 2021

cristianoc72 commented Jul 10, 2021 • edited

gossi commented Jul 10, 2021

cristianoc72 commented Jul 12, 2021

cristianoc72 commented Jul 14, 2021

gossi commented Jul 14, 2021

cristianoc72 commented Aug 5, 2021

gossi commented Aug 5, 2021

gossi commented Aug 5, 2021

LTSCommerce commented Aug 5, 2021

cristianoc72 commented Aug 6, 2021

gossi commented Aug 11, 2021

gossi commented Aug 11, 2021

cristianoc72 commented Aug 11, 2021

cristianoc72 commented Jul 10, 2021 •

edited