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

Huge improve in autocomplete / refactoring / usability for programmers - return specific type instead of generic one #907

Closed
mvorisek opened this issue Feb 3, 2020 · 10 comments · Fixed by #929
Labels

Comments

@mvorisek
Copy link
Member

mvorisek commented Feb 3, 2020

A lot of code constructs look like:

$form = $app->add('Form');

in better case there is an extra type hint like:

use atk4\ui\Form;

/* @var $form Form */
$form = $app->add('Form');

This is a feature request to add __construct option addTo (and maybe some generic option for things like addField) like:

use atk4\ui\Form;

$form = new Form(['addTo' => $app]);

which should be equivalent of $form = $app->add('Form');.

This will hugely improve utocomplete / refactoring / usability for programmers.

All usages and examples should be converted to the new syntax.

$app->add('Form') syntax should be dropped in the new 2.x branch and only $app->add($obj) should be supported. We should aim step by step for 100% refactorability.

@DarkSide666
Copy link
Member

What about $app->add(Form::class) syntax?

@mvorisek
Copy link
Member Author

mvorisek commented Feb 4, 2020

What about $app->add(Form::class) syntax?

These are two separate things. This should be must have everywhere, i.e. to support only FQ class names.

My point is drop support for $app->add(Form::class) (and other very common code usages) to enforce "the object / the top-most" return type as $obj = $app->add(Form::class) returns at best the View type instead of the Form type. For me, the add method is the most affected one with this issue currently and it should be quite easy to fix, wdyt?

User almost everywhere, see https://github.com/atk4/ui/search?l=PHP&q=%22add%28%22

@mvorisek mvorisek changed the title Huge improve in autocomplete / refactoring / usability for programmers Huge improve in autocomplete / refactoring / usability for programmers - return specific type instead of generic one Feb 4, 2020
@georgehristov
Copy link
Collaborator

It is not easy to avoid add method and have the code refactor-ready due to the dynamic nature of PHP which ATK4 employs in full effect.
So it is a balance of both.
You are in effect adding an object definition as array (called a seed). In most cases the object is created by the owner based on provided class name

@mvorisek
Copy link
Member Author

mvorisek commented Feb 4, 2020

@georgehristov I know, but seed is passed to the newly created object (more specifically a factory). So if we pass seed directly to a constructor of the new object and then add it to the parent (via proposed addTo seed), it should be fine - do you agree?

@DarkSide666
Copy link
Member

@mvorisek It should be fine that way I guess, but it will be huge backward-incompatible change. add method is to popular to mess with it :)

@georgehristov
Copy link
Collaborator

The __construct method is intentionally not defined in View as it can take various parameters for each seed. These are the seed array items with numeric keys (1 - N). The 0 is reserved for the class name.
This is what I see so far and unfortunately making the code hint- and refactor- friendly is not trivial.
You can include a View::addTo method but then you need to create the object beforehand.
Using seeds is very convenient for defining objects in a GUI what I believe is ultimately the goal of the ATK team.
I have done that for a project of mine where you basically define the php application in json format in a GUI.

@mvorisek
Copy link
Member Author

mvorisek commented Feb 4, 2020

View::addTo does not make any sense, as if two lines/expressions are needed, we can use simply the $app->add() method.

Proposed steps to implement:

  1. drop support for $app->add() with text seed, allow only to add an object.
  2. consider somehow to implement the addTo option

Before the 2nd step is implemented (and if impossible) the new code should look like:

// if the new object is not needed - refactoring works (i.e. IDE knows we use Form class here)
$app->add(new Form());

// if the new object is needed - refactoring works (i.e. IDE known $form is instanceof Form class)
$form = new Form();
$app->add($form);

// BC break: add should return $this, not the newly created object
$app->add(new Form());

// error: string seed is not supported
$app->add('Form');

@georgehristov, @DarkSide666 wdyt?

@georgehristov
Copy link
Collaborator

String and Array seed must be supported due to reasons I mentioned above

@mvorisek
Copy link
Member Author

mvorisek commented Feb 4, 2020

String and Array seed must be supported due to reasons I mentioned above

Can you provide an example, which can not be converted to:

$obj = new /*class name here*/(/*seed as arguments*/);
$app->add($obj);

@georgehristov
Copy link
Collaborator

You need to be able to take the seed as array, create the object and add it to the owner.

Of course you can convert this to the code you listed but you need to write the complete factory method code every time you'd like to convert the array to an object of the specified class.

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

Successfully merging a pull request may close this issue.

3 participants