nette / php-generator

🐘 Generates neat PHP code for you. Supports new PHP 8.3 features.
https://doc.nette.org/php-generator
Other
2.11k stars 138 forks source link

Option to disable trailing comas after multiline parameters #125

Closed klkvsk closed 1 year ago

klkvsk commented 1 year ago

Version: 4.0.5

Bug Description

When printing multiline arguments, trailing coma is added. This breaks compatibility of generated code with PHP < 8.

Steps To Reproduce

$method = new \Nette\PhpGenerator\Method('foo');
for ($i = 0; $i < 5; $i++) {
    $method->addParameter("veryVeryVeryLongParameter$i");
}
echo (new \Nette\PhpGenerator\Printer)->printMethod($method);

Result:

function foo(
        $veryVeryVeryLongParameter0,
        $veryVeryVeryLongParameter1,
        $veryVeryVeryLongParameter2,
        $veryVeryVeryLongParameter3,
        $veryVeryVeryLongParameter4,
) {
}

Expected Behavior

No coma after $veryVeryVeryLongParameter4.

Possible Solution

Mostly, using trailing comas is a common practice for easier code editing and cleaner diffs, so it should be kept as default. But an option to disable it for compatibility is much needed. Perhaps, with a property/flag passed to Printer.

klkvsk commented 1 year ago

Added a flag in Printer.

But the other case is when parameters are printed within a function call:

$method = new \Nette\PhpGenerator\Method('foo');
$method->addBody('return bar(...?);', [[
    '1234567890-1234567890-1234567890-1234567890',
    '1234567890-1234567890-1234567890-1234567890',
    '1234567890-1234567890-1234567890-1234567890',
]]);

$printer = new \Nette\PhpGenerator\Printer();
echo $printer->printMethod($method);

Output:

function foo()
{
        return bar(
                '1234567890-1234567890-1234567890-1234567890',
                '1234567890-1234567890-1234567890-1234567890',
                '1234567890-1234567890-1234567890-1234567890',
        );
}

This output is done by Dumper (Dumper::dumpArguments). Sure we can pass the flag from Printer to Dumper as done with indentation and wrapLength properties. But, as I see, it's not always being passed. Like in FunctionLike::addBody there's just a new instance of Dumper created without configuration.

Not sure what's the best way to deal with that without global refactoring. Any ideas besides implementing some kind of DumperFactory stuff?

dg commented 1 year ago

Since PHP 7 is no longer supported, I don't want to add support for it which would complicate the code.

I think it will be sufficient to remove the commas with a regular expression, something like preg_replace('~,(\n\t*\))~', '$1', $s)

klkvsk commented 1 year ago

I just noticed that rendered bodies are passed through Printer::indent, where it converts the indentation where needed.
Putting said preg_replace there as the most non-invasive fix.

dg commented 1 year ago

But please do it in your own Printer, I don't want to merge this into the code, because PHP 7 is outdated.

nevmerzhitsky commented 1 year ago

Issues like this (remove or add the comma) may be solved by an additional call of tools like "PHP Coding Standards Fixer" which changes code style, imho.