-
Notifications
You must be signed in to change notification settings - Fork 446
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
Introduce lowercase-string #3438
base: 1.12.x
Are you sure you want to change the base?
Conversation
726e304
to
a776452
Compare
if (count($args) === 0) { | ||
if (count($args) < 2) { | ||
return null; | ||
} | ||
|
||
if (count($args) >= 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this cleanup regarding count-of-args and the indent change over the whole block could be a separate PR to make this diff here more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #3439
edge-case which came to mind while looking at the PR: offset-lookup in a lower-case string yields a lower-case string: |
4b2513c
to
3cbedb0
Compare
There is at least
wich can be improved too, but maybe it's better in another PR. |
What's the use case? Is there an issue linked explaining why we need this? Why not also uppercase string? |
You can see it's used a lot in their codebase https://github.com/search?q=repo%3Avimeo%2Fpsalm%20lowercase-string&type=code It's useful
I personally work a lot with URL recently, and it's useful to unsure/checks that I always work with lowercase string. |
62ef6dd
to
669a302
Compare
Use separate test files in the nsrt/ folder and use a different |
This pull request has been marked as ready for review. |
I think it will be easier and better to have
in another PR. |
I think this should include str-concatenation of 2 lowercase strings, because its a super low level thing |
Indeed, it's supported now. |
I think we are missing a rule test for e.g. /** @param lowercase-string $s */
function doFoo($s) {
if ($s === 'AB') {}
if ('AB' === $s) {}
if ('AB' !== $s) {}
if ($s === 'ab') {}
if ($s !== 'ab') {}
if ($s === 'aBc') {}
if ($s !== 'aBc') {}
if ($s === '01') {}
if ($s === '1e2') {}
if ($s === MyClass::myConst) {}
if ($s === A_GLOBAL_CONST) {}
if (doFoo('hi') === 'AB') {}
} which asserts rule error(s) when comparing constant with lowercase-string. as a followup it also can be useful to test comparisons for string-functions /** @param lowercase-string $s */
function doFoo($s, int $x, int $y) {
$part = substr($s, $x, $y);
if ($part === 'AB) {} // always false
}
|
new StringType(), | ||
new AccessoryNonEmptyStringType(), | ||
]); | ||
|
||
case 'non-empty-lowercase-string': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have a test somewhere for this type?
(I think I see it the very first time today)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 9636e83
7c72c6f
to
9636e83
Compare
After this PR, we may want to introduce check for always false result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- There are no tests about accepting
lowercase-string
. Add a new test method to CallMethodsRuleTest and try passing different things like a constant string, another lowercase-string and general string to a parameter that accepts lowecase-string. - This change is quite disruptive as you can see from the tests that needed changing. We should do this only for bleeding edge, similarly to the list type. This means that all the places that currently do
new IntersectionType
and add the new accesory type will need to doAccessoryLowercaseStringType::intersectWith()
instead. Also, TypeNodeResolver will interpretlowercase-string
type the new way only with bleeding edge.
|
||
public function acceptsWithReason(Type $type, bool $strictTypes): AcceptsResult | ||
{ | ||
if ($type instanceof MixedType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You copied this from accessory literal string type, but it does not belong here.
@@ -1129,19 +1129,19 @@ public function dataArrayDestructuring(): array | |||
'$fourthStringArrayForeachList', | |||
], | |||
[ | |||
'string', | |||
'lowercase-string', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this change. IMHO it should remain a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe it should be numeric-string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'2018-12-19'
is technically a lowercase-string.
And explode($lowercaseString)
is array<lowercase-string>
.
So every element of the array is precised from string
to lowercase-string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: We need excessive tests in TypeCombinatorTest::dataUnion and dataIntersect about the interaction with other string types.
@@ -296,6 +296,11 @@ public function isLiteralString(): TrinaryLogic | |||
return TrinaryLogic::createMaybe(); | |||
} | |||
|
|||
public function isLowercaseString(): TrinaryLogic | |||
{ | |||
return TrinaryLogic::createMaybe(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"10E4" is a numeric string not lowercase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is the reason I kept maybe
Maybe we don't need to do this only for bleeding edgw, just do a better job in VerbosityLevel::getRecommendedLevelByType |
Do you have a preferred solution between bleeding edge and changing getRecommendedLevelByType ? I unsure to understand what is needed to change in |
I'd like for Please start by showing in a test how I think we could bump it from And then in After you do that, the impact on existing rules tests should be minimal. And I could merge it for everyone, not just on bleeding edge. |
9636e83
to
db923b3
Compare
Ok, so If I understood correctly, it should be something like I need to update some tests, but still I have some issue with error message like
because not every Rules use
So I assume such thing need to be changed first... |
9e9ff53
to
48754e1
Compare
For Should I bump it to precise @ondrejmirtes ? Also, do you want me to extract 053e6cd in another PR first ? I dunno if more rule will need the same change. @staabm You're the one who suggested tests for this rule (#3438 (comment)) do you have more rule with lowercase-string in mind ? |
No description provided.