Skip to content

Conversation

devmsh
Copy link
Contributor

@devmsh devmsh commented Jul 22, 2020

By introducing a property for $new_instance, we can easily extract the duplicated code between generators and pull them up to an abstract generator class.

Even If we need to customize any of the extracted methods in the future, we can override them so I change the visibility of these methods to protected.

Hope you will find this useful.

I will start working on the Tree class as we discussed before, and submit a PR tonight 👍

@jasonmccreary
Copy link
Collaborator

I don't understand the purpose of $new_instance. Also, if only statement generators extend from this, I'd prefer to make the abstract class more specific, i.e. StatementGenerator

@devmsh
Copy link
Contributor Author

devmsh commented Jul 22, 2020

@jasonmccreary the $new_instance only use to customize the constructor doc block text, I did not see a big advantage from it, but I tried to refactor the code without any breaking changes.

For the 2nd point, the Generator currently holds the common constructor related logic, but I think it may hold different peace of logic in the next refactoring and some of them may be helpful for other generators?

@jasonmccreary
Copy link
Collaborator

That's fine to leave $new_instance, I'll remove it afterwards.

I understand. However, I am going to change this to StatementGenerator. It in turn can extend an abstract Generator class in a future refactor if necessary.

@devmsh
Copy link
Contributor Author

devmsh commented Jul 22, 2020

Done 👍

@devmsh devmsh force-pushed the refactor-generator branch from 950e6c9 to d27dff2 Compare July 22, 2020 13:14
@jasonmccreary
Copy link
Collaborator

Cool. Thanks. Will review and merge tomorrow.

@jasonmccreary jasonmccreary merged commit 0ea6154 into laravel-shift:master Jul 23, 2020
@devmsh devmsh deleted the refactor-generator branch July 29, 2020 09:48

protected function buildConstructor($statement)
{
static $constructor = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@devmsh what was the point of the static here? It's causing issues with tests in PHP 8.1.

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