support psalm scalar types #112

smoench · 2020-07-16T11:01:11Z

resolves #111

jaapio

In general this looks ok. Thanks for taking the time to create this pr!

I do have a few questions, those are mostly regarding the usage of these types.
Can I use a array-key as a stand alone type?

/** @param array-key $myParam **/
function test($myParam) {}

Or should it always be part of an array, generic or iterable type?

The trait-string and callable-string should extend String_ I think? because they are literally just strings but with a specialized meta type? Maybe we should introduce a special interface for these kind of types. Something like StringLike (I'm open for better names) which is implemented by String_, ClassString, TraitString and CallableString

I would expect that trait-string needs some more information like class-string does? same applies to callable-string?

tests/unit/Types/ArrayKeyTest.php

smoench · 2020-07-17T07:52:09Z

Yes, array-key could be used as a standalone https://psalm.dev/r/746af64896

othercorey · 2020-07-17T16:08:23Z

src/TypeResolver.php

@@ -71,6 +71,7 @@ final class TypeResolver
    private $keywords = [
        'string' => Types\String_::class,
        'class-string' => Types\ClassString::class,
+        'trait-string' => Types\TraitString::class,


What does trait-string provide that class-string does not? class-string is meant to be any class-like fqn.

The goal is not to developp a new feature for PhpDocumentor as much as supporting a notation used by projects out there. If a user has trait-string in his codebase, it's not very useful that we already support class-string

That doesn't answer the question. What is the difference? Why isn't there an interface-trait? Is class-string going to change?

The author said in #111 the goal was to support Psalm's pseudo-types. Here's the list:
https://psalm.dev/docs/annotating_code/type_syntax/scalar_types/

Psalm doesn't support interface-string, so it's not in the list. No change is expected with class-string, trait-string is just a more narrow type that have been created for specific purposes in Psalm

I think we are missing an interface in this lib. So there might be a small change to class-string. But it should not cause any bc breaks since we will just add the interface.

As said before there was a request to support more of psalms pseudo types. Which makes sense since this lib is reflecting the types in docblocks that people write out there. This library is not providing any functionality regarding the types we support.

Most of the supported pseudo types are just meta information to strings. Which do not even exist in php as a language.

class-string resolves the fqsen, so it seems like trait-string should as well.

No, by definition of https://psalm.dev/docs/annotating_code/type_syntax/scalar_types/ trait-string should not be parameterized like class-string. I opened a issue vimeo/psalm#3865 to clarify how trait-string should be used and @orklah has already provided an example https://psalm.dev/r/3bff912a5b

Not everything is about implementing psalm types. Since class-string is essentially the same as trait-string, it makes sense that they would behave similar in TypeResolver. That's what I'm saying.

@jaapio How I should name the interface? StringType or StringableType?

Just dumping some thoughts here:

StaticString because they are representing a value of String_ those types are not really a Type. It is a String_ but with a certain value type.

TypedString

StringRepresentation

StringType should be avoided I think, because it implicates that it is a type..

Maybe they should even extend String_ But I was trying to avoid that, because it would open doors for a large inheritance tree. Which I don't like.

mvriel · 2020-08-24T06:46:45Z

I plan on finalizing #113 this evening or the next and I would love it if the newly introduced pseudotypes here would follow the same pattern. What I have tried to model is that PseudoTypes have an underlying type (so for False, that is boolean) and that you can identify them through an interface.

By extending them from their original Type; you can still typecheck on Type and on the underlying type. This will match the behaviour that a PseudoType is 2 things at the same time. So for false, it is both False and Boolean at the same time

smoench · 2020-08-25T07:41:31Z

Thanks @mvriel. #113 looks promising. Please ping when the changes are done and merged. I will update this PR.

jaapio · 2020-09-02T21:33:23Z

PR #113 is merged, which introduces the new Pseudo types. If you can update this PR according to that approach it would be great!

Regarding the earlier discussion on removing the final keyword from String_ to be able to extend it... this will be the way to go for pseudo-types. And will also allow people to implement their own pseudotypes based on strings. Which is ok.

smoench · 2020-09-18T07:23:52Z

In the meantime psalm has added more pseudo types. They also added interface-string to class-string pragraph so I read it as it should has the same behaviour.

smoench · 2020-09-25T14:11:32Z

@jaapio Can you trigger a retry for the github actions? Thanks in advance!

jaapio · 2020-09-25T14:14:39Z

Done, but looks like it is hanging again 😕

Ha now it is running again

smoench · 2020-09-25T14:32:32Z

Everything has run successfully but the profile workflow 🤷

jaapio · 2020-09-25T14:33:37Z

Please don't mind too much about that. I will check what is going on.

orklah

Could you also add support for the list<?> type? It's basically an array<int, ?>

EDIT: oh, it's not actually a scalar, so it may be out of scope of your PR, nevermind then

orklah · 2020-10-14T17:13:29Z

src/PseudoTypes/CallableString.php

+use phpDocumentor\Reflection\Types\String_;
+
+/**
+ * Value Object representing the type 'string'.


Those comments should be updated with their actual types

jaapio · 2021-02-02T21:11:26Z

A reminder to myself, I need to check this PR.
Code looks good. Just need to check the impact and BC

jaapio

I'm approving this, Code looks good. I didn't find any bc breaks

smoench mentioned this pull request Jul 16, 2020

Support more scalar types of psalm #111

Closed

jaapio requested changes Jul 17, 2020

View reviewed changes

tests/unit/Types/ArrayKeyTest.php Outdated Show resolved Hide resolved

smoench force-pushed the psalm-types branch from a5be03f to bac872b Compare July 17, 2020 10:56

othercorey reviewed Jul 17, 2020

View reviewed changes

smoench force-pushed the psalm-types branch from bac872b to cc9c760 Compare July 24, 2020 13:49

support psalm scalar types

b6a886b

smoench force-pushed the psalm-types branch from cc9c760 to b6a886b Compare September 18, 2020 07:03

smoench marked this pull request as ready for review September 18, 2020 07:09

add interface-string

e675935

smoench force-pushed the psalm-types branch from 9ca72bf to e675935 Compare September 18, 2020 07:29

cs fixes

80a222c

smoench requested a review from jaapio October 7, 2020 06:49

orklah reviewed Oct 14, 2020

View reviewed changes

jaapio approved these changes Jul 24, 2021

View reviewed changes

jaapio merged commit 550e0fb into phpDocumentor:1.x Jul 24, 2021

smoench deleted the psalm-types branch July 24, 2021 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support psalm scalar types #112

support psalm scalar types #112

smoench commented Jul 16, 2020 •

edited

Loading

jaapio left a comment

smoench commented Jul 17, 2020

othercorey Jul 17, 2020

orklah Jul 17, 2020

othercorey Jul 17, 2020

orklah Jul 17, 2020

jaapio Jul 17, 2020

othercorey Jul 17, 2020

smoench Jul 22, 2020

othercorey Jul 22, 2020

smoench Jul 22, 2020

jaapio Jul 22, 2020

mvriel commented Aug 24, 2020

smoench commented Aug 25, 2020

jaapio commented Sep 2, 2020

smoench commented Sep 18, 2020

smoench commented Sep 25, 2020

jaapio commented Sep 25, 2020 •

edited

Loading

smoench commented Sep 25, 2020

jaapio commented Sep 25, 2020

orklah left a comment •

edited

Loading

orklah Oct 14, 2020

jaapio commented Feb 2, 2021

jaapio left a comment

support psalm scalar types #112

support psalm scalar types #112

Conversation

smoench commented Jul 16, 2020 • edited Loading

jaapio left a comment

Choose a reason for hiding this comment

smoench commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvriel commented Aug 24, 2020

smoench commented Aug 25, 2020

jaapio commented Sep 2, 2020

smoench commented Sep 18, 2020

smoench commented Sep 25, 2020

jaapio commented Sep 25, 2020 • edited Loading

smoench commented Sep 25, 2020

jaapio commented Sep 25, 2020

orklah left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaapio commented Feb 2, 2021

jaapio left a comment

Choose a reason for hiding this comment

smoench commented Jul 16, 2020 •

edited

Loading

jaapio commented Sep 25, 2020 •

edited

Loading

orklah left a comment •

edited

Loading