Skip to content

Add strict types for all php files#383

Merged
neenjaw merged 8 commits intoexercism:mainfrom
codedge:#352-strict-types
Jun 29, 2021
Merged

Add strict types for all php files#383
neenjaw merged 8 commits intoexercism:mainfrom
codedge:#352-strict-types

Conversation

@codedge
Copy link
Copy Markdown
Contributor

@codedge codedge commented Jun 27, 2021

Resolve #352

This PR adds declare(strict_types=1) to all php files and an explanation what it means.

The sentences of the explanation are sometimes a bit long to match the 120 chars limit that is enforces with the PSR-2 standard. I therefore made some line breaks to meet this requirement.

Changes

Rule to enfore strict types

An additional phpcs rule enforces a proper declaration of strict types for each file. The rule SlevomatCodingStandard.TypeHints.DeclareStrictTypes from slevomat/coding-standards takes care of this.

Rule to enforce explanation of strict type

I've added a custom phpcs rule, that takes care of the explanation. I think this will make it easier to apply coding standards in the future.

Using composer for lint check

To properly include the custom rules, composer is used to run the check in Github actions.

Specifying php version

To properly specify the version used to CI action, the key php-version needs to be used, instead of version. See shivammathur/setup-php

@neenjaw
Copy link
Copy Markdown
Collaborator

neenjaw commented Jun 27, 2021

If this is WIP can we make it a draft PR? Looks good so far, just needs the explanatory message

@codedge
Copy link
Copy Markdown
Contributor Author

codedge commented Jun 27, 2021

Yes, please make it a draft. I know there are some things open and I'll ping you when I think it is ready.

@neenjaw neenjaw marked this pull request as draft June 27, 2021 15:32
@codedge codedge marked this pull request as ready for review June 27, 2021 16:14
@codedge
Copy link
Copy Markdown
Contributor Author

codedge commented Jun 27, 2021

@neenjaw I feel like the PR is ready to be checked. All changes should be explained in my comment above, I hope 😉

@codedge codedge changed the title WIP: Add strict types for all php files Add strict types for all php files Jun 27, 2021
@neenjaw
Copy link
Copy Markdown
Collaborator

neenjaw commented Jun 27, 2021

Cool, I'll check it out later today

Copy link
Copy Markdown
Collaborator

@neenjaw neenjaw left a comment

Choose a reason for hiding this comment

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

One small comment about the formatting of the paragraph. Looks great otherwise. Tell me what you think and then I can merge?

Comment on lines +4 to +12
* By adding type hints and enabling strict type checking, code can become easier to read,
* self-documenting and reduce the number of potential bugs.
* By default, type declarations are non-strict,
* which means they will attempt to change the original type to match the type specified by the type-declaration.
* In other words, if you pass a string to a function requiring a float,
* it will attempt to convert the string value to a float.
* To enable strict mode, a single declare directive must be placed at the top of the file.
* This means that the strictness of typing is configured on a per-file basis.
* This directive not only affects the type declarations of parameters, but also a function's return type.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there any chance we can format this paragraph differently? It seems strange that each line is so different in length. Maybe aim for a line length of 80? The content itself is fine. :)

@codedge
Copy link
Copy Markdown
Contributor Author

codedge commented Jun 28, 2021

Yes, good idea to go for the 80 chars. I am gonna change it and then you can merge.

@codedge
Copy link
Copy Markdown
Contributor Author

codedge commented Jun 28, 2021

@neenjaw I tried to justify the text a bit more but it is hard to not "destroy" the sentence with unneeded line breaks. Do you think it is ok so?

Comment on lines +3 to +17
/*
* By adding type hints and enabling strict type checking, code can become easier to read,
* self-documenting and reduce the number of potential bugs.
* By default, type declarations are non-strict, which means they will attempt to
* change the original type to match the type specified by the type-declaration.
* In other words, if you pass a string to a function requiring a float,
* it will attempt to convert the string value to a float.
* To enable strict mode, a single declare directive must be placed at the top of the file.
* This means that the strictness of typing is configured on a per-file basis.
* This directive not only affects the type declarations of parameters, but also a function's
* return type.
*
* For more info review the Concept on strict type checking in the PHP track <link>.
* To disable strict typing, comment out the directive below.
*/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/*
* By adding type hints and enabling strict type checking, code can become easier to read,
* self-documenting and reduce the number of potential bugs.
* By default, type declarations are non-strict, which means they will attempt to
* change the original type to match the type specified by the type-declaration.
* In other words, if you pass a string to a function requiring a float,
* it will attempt to convert the string value to a float.
* To enable strict mode, a single declare directive must be placed at the top of the file.
* This means that the strictness of typing is configured on a per-file basis.
* This directive not only affects the type declarations of parameters, but also a function's
* return type.
*
* For more info review the Concept on strict type checking in the PHP track <link>.
* To disable strict typing, comment out the directive below.
*/
/*
* By adding type hints and enabling strict type checking, code can become
* easier to read, self-documenting and reduce the number of potential bugs.
* By default, type declarations are non-strict, which means they will attempt
* to change the original type to match the type specified by the
* type-declaration. In other words, if you pass a string to a function
* requiring a float, it will attempt to convert the string value to a float.
*
* To enable strict mode, a single declare directive must be placed at the top
* of the file. This means that the strictness of typing is configured on a
* per-file basis. This directive not only affects the type declarations of
* parameters, but also a function's return type.
*
* For more info review the Concept on strict type checking in the PHP track
* <link>.
*
* To disable strict typing, comment out the directive below.
*/

This would be a strict 80 width adaptation.

Comment on lines +3 to +17
/*
* By adding type hints and enabling strict type checking, code can become easier to read,
* self-documenting and reduce the number of potential bugs.
* By default, type declarations are non-strict, which means they will attempt to
* change the original type to match the type specified by the type-declaration.
* In other words, if you pass a string to a function requiring a float,
* it will attempt to convert the string value to a float.
* To enable strict mode, a single declare directive must be placed at the top of the file.
* This means that the strictness of typing is configured on a per-file basis.
* This directive not only affects the type declarations of parameters, but also a function's
* return type.
*
* For more info review the Concept on strict type checking in the PHP track <link>.
* To disable strict typing, comment out the directive below.
*/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/*
* By adding type hints and enabling strict type checking, code can become easier to read,
* self-documenting and reduce the number of potential bugs.
* By default, type declarations are non-strict, which means they will attempt to
* change the original type to match the type specified by the type-declaration.
* In other words, if you pass a string to a function requiring a float,
* it will attempt to convert the string value to a float.
* To enable strict mode, a single declare directive must be placed at the top of the file.
* This means that the strictness of typing is configured on a per-file basis.
* This directive not only affects the type declarations of parameters, but also a function's
* return type.
*
* For more info review the Concept on strict type checking in the PHP track <link>.
* To disable strict typing, comment out the directive below.
*/
/*
* By adding type hints and enabling strict type checking, code can become
* easier to read, self-documenting and reduce the number of potential bugs.
* By default, type declarations are non-strict, which means they will attempt
* to change the original type to match the type specified by the
* type-declaration.
*
* In other words, if you pass a string to a function requiring a float,
* it will attempt to convert the string value to a float.
*
* To enable strict mode, a single declare directive must be placed at the top
* of the file.
* This means that the strictness of typing is configured on a per-file basis.
* This directive not only affects the type declarations of parameters, but also
* a function's return type.
*
* For more info review the Concept on strict type checking in the PHP track
* <link>.
*
* To disable strict typing, comment out the directive below.
*/

Here is a strict 80 width adaptation based on your line breaks.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've also tried 120 chars width, but that looks really bad.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(I recommend copying and pasting it in your IDE if you want to preview how it looks)

@neenjaw
Copy link
Copy Markdown
Collaborator

neenjaw commented Jun 28, 2021

Thanks @SleeplessByte, I think one of those formats would be great. @codedge, which formatting do you think works best for this use?

@codedge
Copy link
Copy Markdown
Contributor Author

codedge commented Jun 29, 2021

I change the text to match the second suggestion. Looks better now?

@neenjaw
Copy link
Copy Markdown
Collaborator

neenjaw commented Jun 29, 2021

Looks great, let's merge it

@neenjaw neenjaw added x:module/practice-exercise Work on Practice Exercises x:size/medium Medium amount of work x:status/claimed Someone is working on this issue x:type/content Work on content (e.g. exercises, concepts) labels Jun 29, 2021
@neenjaw neenjaw merged commit 9276d19 into exercism:main Jun 29, 2021
@codedge codedge deleted the #352-strict-types branch June 29, 2021 16:09
beqqrry-aws added a commit to beqqrry-aws/php that referenced this pull request Dec 26, 2023
The PR to add strict types to all PHP files (exercism#383) broke the tests for this assignment because the months are all sent as strings. This PR fixes the tests by changing the months to ints.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

x:module/practice-exercise Work on Practice Exercises x:size/medium Medium amount of work x:status/claimed Someone is working on this issue x:type/content Work on content (e.g. exercises, concepts)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use strict types

3 participants