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

File::getMemberProperties(): minor tweaks after readonly merge #3515

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Dec 20, 2021

Follow up on #3480

I realized after the merge that I had not published a few follow up comments. Addressing those here.

  • The new is_readonly key still needed to be added to the documentation for the File::getMemberProperties() method.
  • For consistency in how the method is tested, adding the is_readonly property to all other test expectations, which makes the testPHP81NotReadonly test case redundant.
  • Adding two additional test cases. In particular I wanted to verify (and safeguard) that the retokenization of the ? to T_NULLABLE after a readonly keyword would not be broken (which it partially was, but that has now been fixed in PHP 8.1 | Tokenizer/PHP: readonly vs union types bug fix #3513).

@jrfnl jrfnl force-pushed the feature/file-getmemberprops-minor-tweaks-for-readonly branch from ee8ea84 to 5c8429d Compare April 20, 2022 10:17
@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 20, 2022

Rebased to get past imaginary merge conflicts.

@gsherwood As this is a follow-up on a PR which has gone into PHPCS 3.7.0 and updates the documentation for that change, can this still be merged in 3.7.0 ?

Follow up on 3480

I realized after the merge that I had not published a few follow up comments. Addressing those here.

* The new `is_readonly` key still needed to be added to the documentation for the `File::getMemberProperties()` method.
* For consistency in how the method is tested, adding the `is_readonly` property to all other test expectations, which makes the `testPHP81NotReadonly` test case redundant.
* Adding two additional test cases. In particular I wanted to verify (and safeguard) that the retokenization of the `?` to `T_NULLABLE` after a `readonly` keyword would not be broken (which it partially was, but that has now been fixed in 3513).
@jrfnl jrfnl force-pushed the feature/file-getmemberprops-minor-tweaks-for-readonly branch from 5c8429d to 9d4c6d1 Compare April 23, 2022 01:55
@gsherwood gsherwood added this to the 3.7.0 milestone May 16, 2022
@gsherwood gsherwood added this to Idea Bank in PHPCS v3 Development via automation May 16, 2022
@gsherwood gsherwood merged commit 502e62a into squizlabs:master May 16, 2022
PHPCS v3 Development automation moved this from Idea Bank to Ready for Release May 16, 2022
@gsherwood
Copy link
Member

Thank you

@jrfnl jrfnl deleted the feature/file-getmemberprops-minor-tweaks-for-readonly branch May 16, 2022 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PHPCS v3 Development
Ready for Release
Development

Successfully merging this pull request may close these issues.

None yet

2 participants