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

Add strict type coding standard #6039

Merged
merged 6 commits into from
Jun 17, 2020
Merged

Add strict type coding standard #6039

merged 6 commits into from
Jun 17, 2020

Conversation

johnsaigle
Copy link
Contributor

@johnsaigle johnsaigle commented Feb 10, 2020

Brief summary of changes

Previous attempts to force type hints or strict typing en masse have failed because it's just too darn complicated to do it all at once.

This introduces a new Strict Type coding standard that we can selectively apply to files by adding to an array in test/run-php-linter.sh. Hopefully this will provide an approach to using stricter typing that is easier to author and review.

Using vendor/bin/phpcbf --standard=test/StrictTypesCS.xml $file will now allow PHPCBF to automatically provide

  • return types
  • param types
  • strict_types =1

based on PHPDocs.

It will also throw errors if Traversable types do not have specific type annotations.

The Database.class.inc file has been modified as an example of the changes

Related

Resolves #5342

@johnsaigle johnsaigle added Category: Feature PR or issue that aims to introduce a new feature Testing PR contains test plan or automated test code (or config files for Travis) labels Feb 10, 2020
@johnsaigle johnsaigle requested a review from driusan February 10, 2020 18:34
@johnsaigle johnsaigle added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Feb 10, 2020
@johnsaigle
Copy link
Contributor Author

johnsaigle commented Feb 10, 2020

Blocked by #5356. Otherwise, when I try to update the composer.lock file it inevitably tries to update phpstan even though I specify only the slevomat/coding-standard package. I think the lock file may be wrong on master.

@driusan
Copy link
Collaborator

driusan commented Feb 10, 2020

#5356 is an issue (which I think is already resolved and needs to be closed?)

@johnsaigle
Copy link
Contributor Author

johnsaigle commented Feb 10, 2020

You're right, I meant #6036. I'll close the issue #5356

@johnsaigle johnsaigle added State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) and removed State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Mar 17, 2020
@cmadjar cmadjar added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jun 12, 2020

<rule ref="SlevomatCodingStandard.TypeHints.LongTypeHints"/>

<rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHintSpacing"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this have to do with enforcing strict types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's already passing and didn't require any changes I guess it's fine, but the PR title says it's for adding strict_type.


<rule ref="SlevomatCodingStandard.TypeHints.NullableTypeForNullDefaultValue"/>

<rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHintSpacing"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Or this?

@johnsaigle johnsaigle removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jun 16, 2020
@johnsaigle johnsaigle requested a review from driusan June 16, 2020 14:25
@driusan driusan closed this Jun 16, 2020
@driusan driusan reopened this Jun 16, 2020
@driusan driusan merged commit 4860cf1 into aces:master Jun 17, 2020
@ridz1208 ridz1208 added this to the 24.0.0 milestone Jun 17, 2020
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
Previous attempts to force type hints or strict typing en masse have failed because it's just too darn complicated to do it all at once.

This introduces a new Strict Type coding standard that we can selectively apply to files by adding to an array in test/run-php-linter.sh. Hopefully this will provide an approach to using stricter typing that is easier to author and review.

Using vendor/bin/phpcbf --standard=test/StrictTypesCS.xml $file will now allow PHPCBF to automatically provide

    return types
    param types
    strict_types =1

based on PHPDocs.

It will also throw errors if Traversable types do not have specific type annotations.

The Database.class.inc file has been modified as an example of the changes

Resolves aces#5342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Feature PR or issue that aims to introduce a new feature Testing PR contains test plan or automated test code (or config files for Travis)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PHPCS] Add sniff for declaration of strict_types
4 participants