Skip to content

[Rector] Apply Rector run on utils/ directory #6225

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 3, 2022
Merged

[Rector] Apply Rector run on utils/ directory #6225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
$rectorConfig->parallel();

// paths to refactor; solid alternative to CLI arguments
$rectorConfig->paths([__DIR__ . '/app', __DIR__ . '/system', __DIR__ . '/tests', __DIR__ . '/utils/Rector']);
$rectorConfig->paths([__DIR__ . '/app', __DIR__ . '/system', __DIR__ . '/tests', __DIR__ . '/utils']);

// do you need to include constants, class aliases or custom autoloader? files listed will be executed
$rectorConfig->bootstrapFiles([
Expand Down
3 changes: 0 additions & 3 deletions utils/PHPStan/CheckFrameworkExceptionInstantiationViaNamedConstructorRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@

final class CheckFrameworkExceptionInstantiationViaNamedConstructorRule implements Rule
{
/**
* @var string
*/
Copy link
Member

@kenjis kenjis Jul 3, 2022

Choose a reason for hiding this comment

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

@samsonasik Why does Rector remove this PHPDoc?

Copy link
Member Author

@samsonasik samsonasik Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is @MGatner request to create custom rule to remove var from constant https://github.com/codeigniter4/CodeIgniter4/blob/develop/utils/Rector/RemoveVarTagFromClassConstantRector.php

Copy link
Member

@MGatner MGatner Jul 4, 2022

Choose a reason for hiding this comment

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

A constant is a constant 🤷‍♂️

I do remember that conversation but I'm also fine if we want to revisit this. I think it is unnecessary to type constants but people do it.

Copy link
Member Author

@samsonasik samsonasik Jul 4, 2022

Choose a reason for hiding this comment

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

ref #4766

Copy link
Member

@kenjis kenjis Jul 4, 2022

Choose a reason for hiding this comment

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

It is a common practice to put @var for constants.
I have never seen removing @var for constants.

But at least, PhpStorm does not need @var for constants.
So it does not seem to be inconvenient if it is removed.

Copy link
Member

@MGatner MGatner Jul 5, 2022

Choose a reason for hiding this comment

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

A constant is no longer a constant.

The underlying values may be mutable but it is still a fixed type at runtime. If for some awful reason somebody used your example I would expect a docblock explanation, not a type. Providing @var typing for static analysis is unnecessary because constants are always strongly-typed to whatever they are.

Copy link
Member

@kenjis kenjis Jul 5, 2022

Choose a reason for hiding this comment

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

Yeah, the type is fixed.
But the value is dynamic determined. Very interesting.
https://3v4l.org/dWAaW
I don't know how and when to use it, though.

Copy link
Member

@MGatner MGatner Jul 5, 2022

Choose a reason for hiding this comment

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

Object constants are strange to me. I can't even find the RFC that allowed them for a good example of why someone would want one 🤷‍♂️ To me the power of a constant is it's fixed, simple value.

Copy link
Member

@MGatner MGatner Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here it is: https://wiki.php.net/rfc/new_in_initializers

Looks like it wasn't deliberately for constants, and is also limited strictly to global constants. The fact that the value for the class constant ends up being dynamic is somewhat of a syntax hack.

Copy link
Member

@MGatner MGatner Jul 5, 2022

Choose a reason for hiding this comment

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

Typed class constants is under discussion for 8.2: https://wiki.php.net/rfc/typed_class_constants

private const ERROR_MESSAGE = 'FrameworkException instance creation via new expression is not allowed, use its named constructor instead';

public function getNodeType(): string
Expand Down
9 changes: 1 addition & 8 deletions utils/PHPStan/CheckUseStatementsAfterLicenseRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,7 @@

final class CheckUseStatementsAfterLicenseRule implements Rule
{
/**
* @var string
*/
private const ERROR_MESSAGE = 'Use statement must be located after license docblock';

/**
* @var string
*/
private const ERROR_MESSAGE = 'Use statement must be located after license docblock';
private const COPYRIGHT_REGEX = '/\* \(c\) CodeIgniter Foundation <admin@codeigniter\.com>/m';

public function getNodeType(): string
Expand Down
28 changes: 13 additions & 15 deletions utils/check_tabs_in_rst.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@
* the LICENSE file that was distributed with this source code.
*/

namespace Utils;

require __DIR__ . '/../system/Test/bootstrap.php';

use CodeIgniter\CLI\CLI;

$rstFilesWithTabs = (string) shell_exec('git grep -EIPn "\t" -- "*.rst"');
$rstFilesWithTabs = explode("\n", $rstFilesWithTabs);
$rstFilesWithTabs = array_map(static function (string $line): array {
preg_match('/^(?P<file>[^:]+):(?P<line>[0-9]+):(?P<code>.+)$/', $line, $matches);
preg_match('/^(?P<file>[^:]+):(?P<line>\d+):(?P<code>.+)$/', $line, $matches);

return [
'file' => $matches['file'],
Expand All @@ -44,20 +46,16 @@
"%s\n\n%s\n",
CLI::color('Tabs in RST files were detected:', 'light_gray', 'red'),
implode("\n", array_map(
static function (string $file, array $parts): string {
return sprintf(
"%s%s\n%s\n",
CLI::color('* in ', 'light_red'),
CLI::color($file, 'yellow'),
implode("\n", array_map(static function (array $line): string {
return sprintf(
'%s | %s',
str_pad($line['line'], 4, ' ', STR_PAD_LEFT),
str_replace("\t", CLI::color('....', 'light_gray', 'red'), $line['code']),
);
}, $parts)),
);
},
static fn (string $file, array $parts): string => sprintf(
"%s%s\n%s\n",
CLI::color('* in ', 'light_red'),
CLI::color($file, 'yellow'),
implode("\n", array_map(static fn (array $line): string => sprintf(
'%s | %s',
str_pad($line['line'], 4, ' ', STR_PAD_LEFT),
str_replace("\t", CLI::color('....', 'light_gray', 'red'), $line['code']),
), $parts)),
),
array_keys($normalizedRstFilesWithTabs),
array_values($normalizedRstFilesWithTabs),
)),
Expand Down