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

allow marking enum cases as deprecated #7192

Merged
merged 3 commits into from Dec 22, 2021

Conversation

pilif
Copy link
Contributor

@pilif pilif commented Dec 20, 2021

This PR will allow an @deprecated annotation to be placed on enum cases which will then cause a DeprecatedConstant issue to be raised when the case is being referenced.

I'm unsure as to whether this is the correct way to do things as I'm not too familiar with the psalm internals, so I'm very happy about pointers in how to do this correctly.

Also, ideally, I would like to have supported the #[Psalm\Deprecated] attribute too, but I don't think the php parser is exposing attributes on enum cases - at least I could not get to them, but maybe I looked in the wrong spot.

@weirdan
Copy link
Collaborator

weirdan commented Dec 20, 2021

Attributes are available on enum cases:

    public $stmts =>
    array(1) {
      [0] =>
      class PhpParser\Node\Stmt\EnumCase#1183 (4) {
        public $name =>
        class PhpParser\Node\Identifier#1182 (2) {
          public $name =>
          string(1) "A"
          protected $attributes =>
          array(4) {
            'startLine' =>
            int(5)
            'startFilePos' =>
            int(50)
            'endLine' =>
            int(5)
            'endFilePos' =>
            int(50)
          }
        }
        public $expr =>
        NULL
        public $attrGroups =>
        array(1) {
          [0] =>
          class PhpParser\Node\AttributeGroup#1181 (2) {
            public $attrs =>
            array(1) {
              [0] =>
              class PhpParser\Node\Attribute#1180 (3) {
                public $name =>
                class PhpParser\Node\Name\FullyQualified#1179 (2) {
                  public $parts =>
                  array(2) {
                    [0] =>
                    string(5) "Psalm"
                    [1] =>
                    string(10) "Deprecated"
                  }

@pilif
Copy link
Contributor Author

pilif commented Dec 20, 2021

thanks. I must have been looking at the wrong spot then (the attrGroups I checked was empty).

I'll check later (tomorrow Europe time - I'm on family duty now) and update the PR

@pilif
Copy link
Contributor Author

pilif commented Dec 21, 2021

yes. you were totally right. the attributes are available.

I have pushed a second commit that adds support for the Psalm\Deprecated and JetBrains\PhpStorm\Deprecated attributes for enum cases.

The patch as shown on GitHub looks a bit crazy because I have extracted the attribute resolution into a new method as it was exactly identical for the property storage too

@pilif pilif changed the title allow marking enum cases as @deprecated allow marking enum cases as deprecated Dec 21, 2021
@pilif
Copy link
Contributor Author

pilif commented Dec 21, 2021

I pushed another commit that updates the docs. However, that fails a test because the documentation samples are checked (very cool) with php 8.0 (not so good).

How should I proceed?

  • update the GitHub Action to parse the tests with php-8.1?
  • remove the updated code sample?
  • add support for version dependent sample codes in the docs? (feels like overkill)

@orklah
Copy link
Collaborator

orklah commented Dec 21, 2021

Let's upgrade that to 8.1, it should not be an issue

@pilif pilif force-pushed the enum-case-deprecation branch 2 times, most recently from 05e12df to 2a33309 Compare December 21, 2021 12:08
@weirdan
Copy link
Collaborator

weirdan commented Dec 21, 2021

Wouldn't adding the issue to the list below suffice?

case 'InvalidEnumBackingType':
case 'InvalidEnumCaseValue':
case 'DuplicateEnumCase':
case 'DuplicateEnumCaseValue':
case 'NoEnumProperties':
$php_version = '8.1';
break;

@weirdan weirdan added the release:feature The PR will be included in 'Features' section of the release notes label Dec 21, 2021
just like with properties, this recognizes both `Psalm\Deprecated` and
`JetBrains\PhpStorm\Deprecated`
@pilif
Copy link
Contributor Author

pilif commented Dec 22, 2021

Wouldn't adding the issue to the list below suffice?

yes. you're right. I noticed only after asking about the general PHP update, so that what I've done. In my rebased commits, I have removed the PHP version change and followed your suggestion.

@weirdan weirdan merged commit f2db139 into vimeo:master Dec 22, 2021
@weirdan
Copy link
Collaborator

weirdan commented Dec 22, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants