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

Restrict the possible values for the casing options under the Portability\ NS #5926

Merged
merged 1 commit into from
Mar 2, 2023
Merged

Restrict the possible values for the casing options under the Portability\ NS #5926

merged 1 commit into from
Mar 2, 2023

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Feb 15, 2023

Q A
Type improvement
Fixed issues n/a

@phansys phansys force-pushed the portability_case branch 8 times, most recently from 0984157 to 4fc43bc Compare February 15, 2023 21:46
@phansys
Copy link
Contributor Author

phansys commented Feb 15, 2023

Could you please help me to resolve the CS issues?

  1. Psalm does not support native constants, so using them in @var, @param and @return tags is producing errors;
  2. I've added @phpstan-* tags in order to resolve the previous issue, but the CS is now complaining about "Incorrect annotations group";
  3. CS marks as error the use of native constants (from the root namespace) like \CASE_LOWER in the PHPDoc annotations, but also complains if they are imported and just used in the tags but not in the code.

Thank you in advance.

@phansys phansys force-pushed the portability_case branch 4 times, most recently from 57b54c8 to 23d8ef0 Compare February 16, 2023 15:54
@phansys phansys marked this pull request as ready for review February 16, 2023 16:07
@derrabus
Copy link
Member

derrabus commented Feb 16, 2023 •
edited
Loading

Psalm does not support native constants

Create class constants as alias for the global constants and reference those in your doc block.

@phansys
Copy link
Contributor Author

phansys commented Feb 16, 2023

Psalm does not support native constants

I've reported vimeo/psalm#9320 to check if it will be supported in the future.

Create class constants as alias for the global constants and reference those in your doc block.

I'll push a new commit with this approach. Thank you!

@phansys phansys force-pushed the portability_case branch 3 times, most recently from 85ffe81 to ed99561 Compare February 16, 2023 18:20
@phansys phansys marked this pull request as draft February 16, 2023 18:21
@phansys phansys marked this pull request as ready for review February 16, 2023 18:30
derrabus
derrabus reviewed Feb 26, 2023
View reviewed changes
psalm.xml.dist Outdated Show resolved Hide resolved
src/Portability/Driver.php Outdated Show resolved Hide resolved
src/Portability/Middleware.php Show resolved Hide resolved
@phansys phansys force-pushed the portability_case branch 5 times, most recently from 6f4f3c0 to 69d2bc2 Compare February 26, 2023 18:01
derrabus
derrabus reviewed Feb 26, 2023
View reviewed changes
tests/Portability/ConnectionTest.php
@@ -18,7 +18,7 @@ public function testGetServerVersion(): void
->method('getServerVersion')
->willReturn('1.2.3');

$connection = new Connection($driverConnection, new Converter(false, false, 0));
$connection = new Connection($driverConnection, new Converter(false, false, Converter::CASE_LOWER));
Copy link
Member

Choose a reason for hiding this comment

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

Wait, why did you change these tests? That shouldn't be necessary, should it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this change is not strictly required, as the previous value (0) is identical to the value provided by this constant. I made these replacements in order to be consistent with the usage of these constants, as I think this also help as an example.
I can revert these changes if you think they are not a proper move for this PR.

Copy link
Member

@derrabus derrabus Feb 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No, you're right. It's better this way, thanks for the explanation. Let's keep the change.

@derrabus derrabus added this to the 3.7.0 milestone Feb 26, 2023
@derrabus
Copy link
Member

LGTM. I'll merge this change as soon as we create the 3.7.x branch. Thank you very much.

@derrabus derrabus changed the title [SCA] Restrict the possible values for the casing options under the Portability\ NS Restrict the possible values for the casing options under the Portability\ NS Mar 2, 2023
@derrabus derrabus changed the base branch from 3.6.x to 3.7.x March 2, 2023 19:28
@derrabus derrabus merged commit 762d8bf into doctrine:3.7.x Mar 2, 2023
@phansys phansys deleted the portability_case branch March 2, 2023 19:33
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Portability Static Analysis
Projects
None yet
Milestone
3.7.0
Development

Successfully merging this pull request may close these issues.
2 participants
@phansys @derrabus