Skip to content

Minor project style improvements #129

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

Draft
wants to merge 5 commits into
base: 1.5.x
Choose a base branch
from
Draft

Minor project style improvements #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8663d86
Add use statement to bin
mvorisek Jun 6, 2024
2bb4ac7
Unify use statements placement in examples
mvorisek Jun 6, 2024
60281fa
"tests/performance.php" file was removed
mvorisek Jun 7, 2024
5a38ae5
Add bin file to phpcs
mvorisek Jun 7, 2024
218a72a
Remove historical comments
mvorisek Jun 3, 2024
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
6 changes: 4 additions & 2 deletions bin/sql-formatter
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#!/usr/bin/env php
<?php

if("cli" !== php_sapi_name()) {
use Doctrine\SqlFormatter\SqlFormatter;

if('cli' !== php_sapi_name()) {
Copy link
Member

Choose a reason for hiding this comment

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

This kind of CS change should be handled by PHPCS. Can we please configure PHPCS so that it runs on the files you've changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greg0ire you are an CI expect, might I know why the phpcs is not working for examples/ and for bin/?

examples/ - they are added in https://github.com/doctrine/sql-formatter/blob/e1e39d8f4c/phpcs.xml.dist#L22 and ending with .php, but it seems they are completely ignored as for ex. constants like in https://github.com/doctrine/sql-formatter/blob/e1e39d8f4c/examples/cli.php#L10 are not added to uses.

bin/ - the file does not end with .php but a full path is specified - it seems ignored currently as well as I would expect the php_sapi_name() in https://github.com/doctrine/sql-formatter/blob/e1e39d8f4c/bin/sql-formatter#L6 to be replaced with a constant.

Copy link
Member

Choose a reason for hiding this comment

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

It works, it's just that you did not spot

sql-formatter/phpcs.xml.dist

Lines 25 to 36 in 7c0096a

<rule ref="Generic.Files.InlineHTML">
<exclude-pattern>examples/*.php</exclude-pattern>
<exclude-pattern>tests/performance.php</exclude-pattern>
</rule>
<!-- The sniff does not detect usages for some reason -->
<rule ref="SlevomatCodingStandard.Namespaces.UnusedUses">
<exclude-pattern>examples/*.php</exclude-pattern>
</rule>
<!-- The sniff attempts to badly format templating syntax end blocks -->
<rule ref="SlevomatCodingStandard.ControlStructures.BlockControlStructureSpacing.IncorrectLinesCountAfterControlStructure">
<exclude-pattern>examples/*.php</exclude-pattern>
</rule>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spot the config but found UnusedUses rule not relevant...

So in examples/ - InlineHTML is still preventing any CS rules when a single inline HTML/PHP is found?

And in bin/ the CS rules are still not applied.

Copy link
Member

Choose a reason for hiding this comment

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

Well in bin, there are no files ending with .php 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

InlineHTML is still preventing any CS rules when a single inline HTML/PHP is found?

You don't need a working Github CI to figure that out, do you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well in bin, there are no files ending with .php 🤷‍♂️

In the config, I am using <file>bin/sql-formatter</file>, so I wonder how to do that, I need a help in this topic, IDK the solution myself.

Copy link
Member

Choose a reason for hiding this comment

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

Consider filing an issue at https://github.com/PHPCSStandards/PHP_CodeSniffer/

echo "<p>Run this PHP script from the command line to see CLI syntax highlighting and formatting. It supports Unix pipes or command line argument style.</p>";
echo "<pre><code>php bin/sql-formatter \"SELECT * FROM MyTable WHERE (id>5 AND \\`name\\` LIKE \\&quot;testing\\&quot;);\"</code></pre>";
echo "<pre><code>echo \"SELECT * FROM MyTable WHERE (id>5 AND \\`name\\` LIKE \\&quot;testing\\&quot;);\" | php bin/sql-formatter</code></pre>";
Expand All @@ -27,4 +29,4 @@ foreach ($autoloadFiles as $autoloadFile) {
}
}

echo (new \Doctrine\SqlFormatter\SqlFormatter())->format($sql);
echo (new SqlFormatter())->format($sql);
10 changes: 5 additions & 5 deletions examples/cli.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

declare(strict_types=1);

use Doctrine\SqlFormatter\SqlFormatter;
use Doctrine\SqlFormatter\Tokenizer;

require_once __DIR__ . '/../vendor/autoload.php';

if (PHP_SAPI !== 'cli') :?>
<p>
Run this php script from the command line to see CLI syntax highlighting and
Expand All @@ -23,11 +28,6 @@
$sql = stream_get_contents($fp);
}

require_once __DIR__ . '/../vendor/autoload.php';

use Doctrine\SqlFormatter\SqlFormatter;
use Doctrine\SqlFormatter\Tokenizer;

assert($sql !== false);

echo (new SqlFormatter())->format($sql);
4 changes: 2 additions & 2 deletions examples/examples.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@

declare(strict_types=1);

require_once __DIR__ . '/../vendor/autoload.php';

use Doctrine\SqlFormatter\NullHighlighter;
use Doctrine\SqlFormatter\SqlFormatter;
use Doctrine\SqlFormatter\Tokenizer;

require_once __DIR__ . '/../vendor/autoload.php';

$formatter = new SqlFormatter();

// Example statements for formatting and highlighting
Expand Down
2 changes: 1 addition & 1 deletion phpcs.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@

<rule ref="Doctrine"/>

<file>bin/sql-formatter</file>
<file>examples</file>
<file>src</file>
<file>tests</file>

<rule ref="Generic.Files.InlineHTML">
<exclude-pattern>examples/*.php</exclude-pattern>
<exclude-pattern>tests/performance.php</exclude-pattern>
</rule>
<!-- The sniff does not detect usages for some reason -->
<rule ref="SlevomatCodingStandard.Namespaces.UnusedUses">
Expand Down
3 changes: 0 additions & 3 deletions src/Tokenizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,7 +849,6 @@ private function makeRegexFromList(array $values, bool $sorted = false): string

/**
* Takes a SQL string and breaks it into tokens.
* Each token is an associative array with type and value.
*
* @param string $string The SQL string
*/
Expand Down Expand Up @@ -881,8 +880,6 @@ public function tokenize(string $string): Cursor
* @param string $string The SQL string
* @param string $upper The SQL string in upper case
* @param Token|null $previous The result of the previous createNextToken() call
*
* @return Token An associative array containing the type and value of the token.
*/
private function createNextToken(string $string, string $upper, int $offset, Token|null $previous = null): Token
{
Expand Down