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

Replace object to stringable #747

Closed
wants to merge 3 commits into from
Closed

Replace object to stringable #747

wants to merge 3 commits into from

Conversation

kriptogenic
Copy link

@kriptogenic kriptogenic commented Aug 20, 2023

Resolves #738

@what-the-diff
Copy link

what-the-diff bot commented Aug 20, 2023

PR Summary

  • Enhancement to Constraint.php
    The 'use Stringable' enhancement has been added to src/Constraint/Constraint.php, allowing the code to work with any variable as if it's a string, which simplifies operation.

  • Alterations to name property and methods in Constraint.php
    The type specification of name property and related methods (getName and name) has changed from accepting string, null, or object to accepting Stringable interface, string, or null. This makes the types more compatible with Stringable objects, thus ensuring smoother operations and lesser errors.

  • New Test Added to ConstraintTest.php
    A new test has been added to confirm the modification of the getName method of the constraint instance, this test uses an anonymous class that implements the Stringable interface which will help verify that the changes are effective and error-free.

@codecov
Copy link

codecov bot commented Aug 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -6.16% ⚠️

Comparison is base (8eb3a7c) 99.73% compared to head (7cceab4) 93.58%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #747      +/-   ##
============================================
- Coverage     99.73%   93.58%   -6.16%     
  Complexity     1269     1269              
============================================
  Files            63       63              
  Lines          3038     3038              
============================================
- Hits           3030     2843     -187     
- Misses            8      195     +187     
Files Changed Coverage Δ
src/Constraint/Constraint.php 100.00% <100.00%> (ø)

... and 13 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kriptogenic
Copy link
Author

I see there are some failed tests.
In Yiisoft\Db\Tests\Provider\SchemaProvider: AnyValue::getInstance() used as name, which is object. I've to options for fixing it.

  1. Create new class AnyStringableValue which extends AnyValue and implements __toString()
  2. Just rename AnyValue to AnyStringableValue and implement __toString, because this class only used for setting name() method

In Yiisoft\Db\Tests\Common\CommonSchemaTest: AnyCaseValue object used as name, which can include array or string value. I didn't understand that test case. What should I do with this test case?

P.S. I want to start contribute with this small pull-request. I will be happy for any suggestion. Don't judge strictly :)

@samdark
Copy link
Member

samdark commented Aug 26, 2023

First option would be backwards compatible. You can mark AnyValue as @deprecated.

@samdark
Copy link
Member

samdark commented Aug 26, 2023

Also see #738 (comment). That's a good question.

@kriptogenic kriptogenic closed this by deleting the head repository Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace object type for constraint name with Stringable
3 participants