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

change somes things for SA #212

Merged
merged 1 commit into from
Feb 22, 2020
Merged

Conversation

orklah
Copy link
Contributor

@orklah orklah commented Feb 21, 2020

Change some things:

  • Replace StaticMethod by Tag in docs
  • standardize create() return type (self or ?self)
  • change if(strlen($x)) to if($x === '')
    and some other little fixes

@orklah
Copy link
Contributor Author

orklah commented Feb 21, 2020

https://github.com/phpDocumentor/ReflectionDocBlock/pull/212/checks?check_run_id=461109833#step:5:30
I'm pretty sure ?self is covariant with ?Tag...

Should I revert this?

@jaapio
Copy link
Member

jaapio commented Feb 21, 2020

Stricly the change from ?Tag to ?Self is a breaking change since you are narrowing the return type. But I do agree that isn't a break in this specific case.

php 7.4 would even allow this native, so I think it is ok to leave it. But I'm wondering if we can make the build green again after we are merging this :-)

@orklah
Copy link
Contributor Author

orklah commented Feb 21, 2020

I'm narrowing the declared type, but it will return the same content. Anyway, as for last time, the class is final and can't be inherited so the declared type doesn't even matter :)

I'm afraid you won't be able to have green build after this too. The obvious solution would be a new major but it's kinda overkill just for a return type.

Very big projects would maintain more than one version and work on next major while still maintaining the current versions but this is not really feasible here. I guess we're left with a "TODO list" issue with every breaking change that could be made on next major?

@jaapio
Copy link
Member

jaapio commented Feb 21, 2020

Except that this is not a bc break. So it might end up in a todo list and a bug report on the tool that does the checks. Again 😊

@orklah orklah mentioned this pull request Feb 22, 2020
@GrahamCampbell
Copy link
Contributor

 Problem parsing /github/workspace/psalm.xml:
  Error on line 33:
    Element '{https://getpsalm.org/schema/config}referencedClass': This element is not expected.

@orklah
Copy link
Contributor Author

orklah commented Feb 22, 2020

 Problem parsing /github/workspace/psalm.xml:
  Error on line 33:
    Element '{https://getpsalm.org/schema/config}referencedClass': This element is not expected.

Thanks :)

This Psalm version is kinda old, I'll change the ignore rule, this one doesn't recognize ignored classes

psalm.xml Outdated
@@ -30,7 +30,7 @@
<DeprecatedInterface>
<errorLevel type="info">
<!-- Will be removed in 6.0.0 issues/211 -->
<referencedClass name="phpDocumentor\Reflection\DocBlock\Tags\Factory\StaticMethod"/>
<referencedClass name="phpDocumentor/Reflection/DocBlock/Tags/Factory/StaticMethod"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you need \\?

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, I think this was a wrong fix. I make the "windows directory separator" a lot in psalm, but this is a class name, not a path. I believe Psalm was screaming because referencedClass was introduced in a newer version. (IIRC, the version used in CI is 3.0.X)

Copy link
Member

Choose a reason for hiding this comment

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

Let me check, I found a way to install a newer psalm version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check, I found a way to install a newer psalm version

Nice!

@jaapio
Copy link
Member

jaapio commented Feb 22, 2020

please rebase against master, this will update psalm, the build is currently failing but I think that's ok for now since you are working on this :-)

@orklah
Copy link
Contributor Author

orklah commented Feb 22, 2020

This should be good to merge. I reverted the detected BC break for now

@jaapio jaapio merged commit cd72d39 into phpDocumentor:master Feb 22, 2020
@orklah orklah deleted the static-fixes branch February 22, 2020 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants