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.

Sign up for GitHub

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

Jump to bottom

Convert hardcoded strings to hintable fields in Contains{One, Many}Test #822

Merged
merged 3 commits into from
Jan 1, 2021
Merged

Convert hardcoded strings to hintable fields in Contains{One, Many}Test #822

merged 3 commits into from
Jan 1, 2021

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Dec 30, 2020 •
edited
Loading

related with #800

mvorisek
mvorisek commented Dec 30, 2020
View reviewed changes
tests/ContainsManyTest.php Outdated
@@ -19,8 +19,9 @@
/**
* Invoice model.
*
* @property Line2 $lines @Atk\RefMany()
* @property string $total_gross @Atk\Field()
* @property Line2 $lines @Atk\RefOne()
Copy link
Member Author

@mvorisek mvorisek Dec 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

always use RefOne, RefMany should be used once load() returns another instance (otherwise you will not be able to laod a model, you must get it thru iterator first)

mvorisek
mvorisek commented Dec 30, 2020
View reviewed changes
tests/ContainsManyTest.php Outdated

// total_gross - calculated by php callback not by SQL expression
$this->addCalculatedField($this->fieldName()->total_gross, function (self $m) {
$total = 0;
foreach ($m->ref('lines') as $line) {
$total += $line->get('total_gross');
foreach ($m->lines as $line) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is one strong advantage of hintable magic, thanks to #821 foreach value has correct type for phpstan as the iterated model

@mvorisek mvorisek force-pushed the more_hintable branch 2 times, most recently from a73db73 to c87335f Compare December 30, 2020 23:59
@mvorisek mvorisek added the MAJOR label Dec 31, 2020
@mvorisek mvorisek changed the title Convert hardcoded strings to hintable fields Convert hardcoded strings to hintable fields in ContainsOneTest/ContainsManyTest Dec 31, 2020
@mvorisek mvorisek removed the MAJOR label Dec 31, 2020
@mvorisek mvorisek changed the title Convert hardcoded strings to hintable fields in ContainsOneTest/ContainsManyTest Convert hardcoded strings to hintable fields in Contains{One, Many}Test Dec 31, 2020
mvorisek
mvorisek commented Dec 31, 2020
View reviewed changes
src/Model/ReferencesTrait.php
@@ -80,7 +80,7 @@ public function addRef(string $link, array $defaults): Reference
*
* @return Reference\HasOne
*/
public function hasOne(string $link, array $defaults = []): Reference
public function hasOne(string $link, array $defaults = []) //: Reference
Copy link
Member Author

Choose a reason for hiding this comment

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

otherwise autohinting (from phpdoc) does not work

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add HasOne to Method Signature?

public function hasOne(string $link, array $defaults = []): Reference\HasOne

Copy link
Member Author

@mvorisek mvorisek Jan 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

because it can be implemented by another class not based on this one

this is almost the no1 issue with Atk

Copy link
Contributor

Choose a reason for hiding this comment

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

so you mean a child class could e.g. implement
public function hasOne(string $link, array $defaults = []): Some\Other\Reference
and that would fail if we set the parent signature to Reference\HasOne?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

grml this is ugly. Do you really think adding Reference\HasOne would be an issue? I see return types in method signatures as an important part of good code. While this PR helps IDEs and phpstan and so on using comments, it weakens the method signatures which was something I liked about almost any of your PR: You created stricter, clearer method signatures.

@mvorisek mvorisek force-pushed the more_hintable branch 2 times, most recently from 26edb56 to 781497d Compare December 31, 2020 00:26
@mvorisek mvorisek force-pushed the more_hintable branch 2 times, most recently from 0032e48 to 5f9863c Compare December 31, 2020 11:49
@mvorisek mvorisek marked this pull request as ready for review December 31, 2020 11:51
@mvorisek mvorisek added the RTM label Dec 31, 2020
@mvorisek mvorisek merged commit 91f6148 into develop Jan 1, 2021
@mvorisek mvorisek deleted the more_hintable branch January 1, 2021 14:10
@mvorisek mvorisek restored the more_hintable branch January 20, 2021 13:29
@mvorisek mvorisek deleted the more_hintable branch January 20, 2021 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhilippGrashoff PhilippGrashoff PhilippGrashoff left review comments

@georgehristov georgehristov georgehristov approved these changes

@DarkSide666 DarkSide666 Awaiting requested review from DarkSide666

Assignees
No one assigned
Labels
Refactoring RTM
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mvorisek @georgehristov @PhilippGrashoff