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

NEW Add sniffer code #1

Merged
merged 6 commits into from
Nov 13, 2023

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Oct 31, 2023

NOTES

  1. After this is merged, we need to add it to packagist and set up the relevant webhook(s).
  2. The CI builds on the cc fork are failing, but they pass locally. I think it's related to MNT Add new repo so installer isn't included gha-generate-matrix#70

Issue

src/Sniffer.php Outdated
Comment on lines 229 to 250
foreach ($todo as $block) {
if ($block->ignored === false) {
$currDir = dirname($block->realPath);
if ($lastDir !== $currDir) {
if (PHP_CODESNIFFER_VERBOSITY > 0) {
echo 'Changing into directory ' . Common::stripBasepath($currDir, $sniffer->config->basepath) . PHP_EOL;
}

$lastDir = $currDir;
}

$sniffer->processFile($block);
} else if (PHP_CODESNIFFER_VERBOSITY > 0) {
echo 'Skipping ' . basename($block->path) . PHP_EOL;
}

$numProcessed++;
$sniffer->printProgress($block, $numBlocks, $numProcessed);
}
Copy link
Member Author

@GuySartorelli GuySartorelli Oct 31, 2023

Choose a reason for hiding this comment

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

For this method, everything outside this loop is copied directly from Runner::run(), so we probably don't want to change much of that.

src/Sniffer.php Outdated Show resolved Hide resolved
@GuySartorelli GuySartorelli force-pushed the pulls/1/new-repo branch 4 times, most recently from 61ab06c to 58b240b Compare October 31, 2023 23:44
Comment on lines +37 to +45
For some reason this one doesn't get auto-fixed.

- this one will be indented
```php
class FinalClass {
private string $lastProperty='this is the value';
}
```
- And it'll also have linting failures
Copy link
Member Author

@GuySartorelli GuySartorelli Nov 1, 2023

Choose a reason for hiding this comment

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

The note about this one not getting auto-fixed is obviously pointing out a bug.

Once this PR is merged I'll raise a new issue about it, but I couldn't see an obvious way to resolve it. I figured it was better to have this working in the 90% of scenarios that it needs to for now, and we can fix that later on.

It does get correctly linted (there's another test validating that), so we will be able to fix those linting failures manually in the meantime.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

I haven't tested, I've just done a fairly quick code scan over this

Was some of this code lifted and shifted from PHPCS? If so can you point out those bits so I can largely ignore them

src/CodeBlock.php Outdated Show resolved Hide resolved
src/CodeBlock.php Show resolved Hide resolved
src/Sniffer.php Outdated Show resolved Hide resolved
src/Sniffer.php Outdated Show resolved Hide resolved
src/Sniffer.php Outdated Show resolved Hide resolved
src/Sniffer.php Outdated Show resolved Hide resolved
src/Sniffer.php Show resolved Hide resolved
src/Sniffer.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member Author

Was some of this code lifted and shifted from PHPCS? If so can you point out those bits so I can largely ignore them

As discussed in person, I've pointed the main blocks of these out with PR comments, plus the FixerReport is basically a lift-and-shift with some parts removed.

There are other bits here-and-there but not in such easily identifiable chunks.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Need to use standard namespaces

src/CodeBlock.php Outdated Show resolved Hide resolved
@emteknetnz emteknetnz merged commit 47f0c4d into silverstripe:1 Nov 13, 2023
@emteknetnz emteknetnz deleted the pulls/1/new-repo branch November 13, 2023 00:06
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.

2 participants