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

Add hintable fields support #800

Merged
merged 2 commits into from
Dec 11, 2020
Merged

Add hintable fields support #800

merged 2 commits into from
Dec 11, 2020

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Dec 9, 2020 •
edited
Loading

see how to use in atk4/ui#1559

then we can convert all tests to it

@DarkSide666
Copy link
Member

What license is for mahalux/atk4-hintable ?

@mvorisek
Copy link
Member Author

What license is for mahalux/atk4-hintable ?

free for use as lib, donations welcomed if helpful for corporate clients :)

@mvorisek mvorisek force-pushed the add_hintable_trait branch 7 times, most recently from ed7f311 to 937c220 Compare December 11, 2020 07:46
DarkSide666
DarkSide666 reviewed Dec 11, 2020
View reviewed changes
tests/ContainsManyTest.php Outdated
foreach ($m->ref('lines') as $line) {
$total += $line->get('total_gross') * $line->get('discounts_percent') / 100;
foreach ($m->lines as $line) {
$total += $line->total_gross * $line->get('discounts_percent') / 100;
Copy link
Member

Choose a reason for hiding this comment

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

could be also $line->discounts_percent, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course! About 1000 more replaces ;-)

Working with PHPStan team to fix Model iterator support, then probably replace everything with Rector.

Copy link
Member Author

Choose a reason for hiding this comment

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

discussion here: phpstan/phpstan#4220 (reply in thread)

DarkSide666
DarkSide666 approved these changes Dec 11, 2020
View reviewed changes
Copy link
Member

@DarkSide666 DarkSide666 left a comment

Choose a reason for hiding this comment

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

Real magic, but looks good to use and interesting as a bit different coding example.
Also important that it's 100% BC, so if you don't want to use this magic in your code - no worries.

@georgehristov
Copy link
Collaborator

Really good @mvorisek.
My only comment would be that it is preferrable to have this into data directly to avoid dependency on external package.

@DarkSide666
Copy link
Member

Really good @mvorisek.
My only comment would be that it is preferrable to have this into data directly to avoid dependency on external package.

Probably not in data directly, but in atk4/core because we can maybe use similar approach for atk4/ui controls too.

@mvorisek mvorisek merged commit b67712c into develop Dec 11, 2020
@mvorisek mvorisek deleted the add_hintable_trait branch December 11, 2020 14:22
@mvorisek mvorisek added the MAJOR label Dec 11, 2020
@mvorisek mvorisek mentioned this pull request Dec 31, 2020
Merged
@mvorisek mvorisek mentioned this pull request Jan 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DarkSide666 DarkSide666 DarkSide666 approved these changes

@georgehristov georgehristov Awaiting requested review from georgehristov

Assignees
No one assigned
Labels
MAJOR
Milestone
No milestone
Development

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