consolidation / output-formatters

Apply transformations to structured data to write output in different formats.
Other
192 stars 13 forks source link

Add optional var_dump formatter #65

Closed Chi-teck closed 6 years ago

Chi-teck commented 6 years ago

Summary

Symfony VarDumper component offers a nice way to dump variables into a terminal. This would be handy to have this as a replacement to var_export format when that component is installed.

greg-1-anderson commented 6 years ago

Looks okay in concept. Could you add var_dumper to require-dev and suggests, and add a simple test?

Current test failures are caused by a PSR-2 code style issue. composer cbf should fix it.

Chi-teck commented 6 years ago

@greg-1-anderson, why does this package need composer.lock file?

Chi-teck commented 6 years ago

Note that we could write data using $output->writeln like shown below. This would be consistent with other formatters. The only concern is support for colors. With the current implementation, VarDumper is able to determine if the output supports colors. Dumping data to a string requires setting support for colors explicitly which, I take it, may cause some problems in environments that do not support colors.

public function write(OutputInterface $output, $data, FormatterOptions $options)
{
    $dumper = new CliDumper();
    $dumper->setColors(TRUE);
    $cloned_data = (new VarCloner())->cloneVar($data);
    $output->writeln($dumper->dump($cloned_data, true));
}
greg-1-anderson commented 6 years ago

The other problem here is that the tests are failing under php 5.4, because symfony/var-dumper requires php 5.5. The easiest solution to this problem would be to simply remove symfony/var-dumper from the Symfony 2 test. However, I think it would require enhancing the composer-test-scenarios project in order to do this.

As for why there is a lock file at all, see Highest / Lowest Testing with Multiple Symfony Versions for a discussion on this topic.

greg-1-anderson commented 6 years ago

To do the php 5.4 test, in the composer.json file, change

"create-scenario symfony2 'symfony/console:^2.8' --platform-php '5.4' --no-lockfile"

to:

"create-scenario symfony2 'symfony/console:^2.8' --platform-php '5.4' --no-lockfile --remove symfony/var-dumper"

You'll need to test with "greg-1-anderson/composer-test-scenarios": "dev-remove-project" in order for the --remove option to work. You will also need to mark the var-dumper test skipped if symfony/var-dumper is not available (or perhaps it would be better to mark the test skipped if the php version is 5.4, to avoid false positives if symfony/var-dumper is inadvertently omitted in other versions of php).

Chi-teck commented 6 years ago

Stream output is supported conditionally. Tests are implemented the same way as for var_export.

You'll need to test with "greg-1-anderson/composer-test-scenarios": "dev-remove-project" in order for the --remove option to work.

That's not clear. Do you want to use remove-project branch of composer-test-scenarios for this project?

greg-1-anderson commented 6 years ago

I'd like to see the tests pass here on the remove-project branch of composer-test-scenarios. Once that happens, I'll make a stable tag and we can go back to ^1 in the composer.json here.

Chi-teck commented 6 years ago

Turns out VarDumper:2.8 does not support getting dump through return value. https://travis-ci.org/consolidation/output-formatters/jobs/329980565#L595

greg-1-anderson commented 6 years ago

Do you know why the composer verify is failing in the php 5.5 test? Seems like your composer.lock should be up-to-date, but you might want to double-check this by running composer verify locally.

As for the php 5.4 test, it would be fine with me if you wanted to go back to removing VarDumper for that test, like https://github.com/consolidation/output-formatters/pull/65/commits/205da6e801f95e8644109ca2bccb152b885e420e. The inconvenient thing about doing that is that some unrelated tests then fail, because var_dump does not appear in the list of supported formats. The way I usually work around edge cases like this is to just simplify the actual output to remove optional / variable output that the test does not care about, e.g.:

$actual = str_replace(',var_dump','', $actual);

Chi-teck commented 6 years ago

Seems like your composer.lock should be up-to-date, but you might want to double-check this by running composer verify locally.

Did you mean composer validate? It works locally.

$ composer validate
./composer.json is valid, but with a few warnings
See https://getcomposer.org/doc/04-schema.md for details on the schema
symfony/console is required both in require and require-dev, this can lead to unexpected behavior

Anyway, I did composer update and pushed composer.lock file and scenarios. The problem seems resolved now.

As of supporting VarDumper v2, I replaced return value with memory stream to get the dump. This way works fine in VarDumper v3+ as well. In fact, VarDumper v3+ uses this approach internally when the return value is enabled.

Chi-teck commented 6 years ago

It worth noting since the tests rely on buffered output colored dumps have no test coverage.

greg-1-anderson commented 6 years ago

Merged via commandline to fix conflicts.

Thanks!