-
Notifications
You must be signed in to change notification settings - Fork 614
Add option to print all paths as absolute paths #1620
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
Conversation
2713023
to
62db681
Compare
@@ -83,6 +84,7 @@ protected function getDefaultExtensions() | |||
new AutoloaderExtension(['' => '%paths.base%/features/bootstrap']), | |||
new SuiteExtension($processor), | |||
new OutputExtension('pretty', $this->getDefaultFormatterFactories($processor), $processor), | |||
new PathOptionsExtension(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a new extension that adds the new path options
*/ | ||
public function __construct( | ||
ResultToStringConverter $resultConverter, | ||
ExceptionPresenter $exceptionPresenter, | ||
TranslatorInterface $translator, | ||
$basePath, | ||
string $basePath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter is not used any more because the configurable path printer service takes care of handling path printing
@@ -281,35 +284,19 @@ private function printStepStat( | |||
$printer->writeln(); | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now this function is not needed
@@ -220,6 +220,7 @@ protected function loadCorePrinters(ContainerBuilder $container) | |||
new Reference('output.node.printer.pretty.width_calculator'), | |||
'%paths.base%', | |||
]); | |||
$definition->addMethodCall(('setConfigurablePathPrinter'), [new Reference('configurable.path.printer')]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We inject the new service into the classes that need it
@@ -102,6 +106,13 @@ public function withPrintUnusedDefinitions(bool $printUnusedDefinitions = true): | |||
return $this; | |||
} | |||
|
|||
public function withPathOptions(bool $printAbsolutePaths): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New config option to set this option using PHP
@@ -215,6 +227,22 @@ private function addUnusedDefinitionsToExpr(Expr &$expr): void | |||
} | |||
} | |||
|
|||
private function addPathOptionsToExpr(Expr &$expr): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And new yaml to php converter for this option
|
||
namespace Behat\Testwork\PathOptions\Printer; | ||
|
||
final class ConfigurablePathPrinter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new service that prints paths, adding an option to decide if they should be printed as relative (default) or absolute
62db681
to
5ab8b7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be great @carlos-granados, thanks. Just a couple of thoughts.
$this->basePath = $basePath; | ||
} | ||
|
||
public function setConfigurablePathPrinter(ConfigurablePathPrinter $configurablePathPrinter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little nervous that there might still be a BC impact with some of these classes if they're used by extensions / formatters and:
- They aren't taking them through our service container, so this method doesn't get called and class fails attempting to call the
->configurablePathPrinter
- They are using our service container definitions, but decorating our class, so this method won't be defined on their decorator and the service container will throw on attempting to create the service.
I wonder if we can make this safer.
Perhaps if we instead add this as an optional constructor parameter with a fallback instead of a setter - e.g. like this:
public function __construct(
private ResultToStringConverter $resultConverter,
private ExceptionPresenter $exceptionPresenter,
private TranslatorInterface $translator,
string $basePath,
?ConfigurablePathPrinter $configurablePathPrinter = null
) {
$this->configurablePathPrinter = $configurablePathPrinter ?? new ConfigurablePathPrinter($basePath, printAbsolutePaths: false);
}
Then I think:
- If people are doing
new ListPrinter(...)
it will continue to work exactly as now, it just won't respect the new config option - If people have decorated our class without changing the constructor args, they'll get an extra argument that is not in their constructor signature and ignore it
- If people have decorated our class and changed the constructor args, they will get the same class / args as before
What do you think?
I think this will also help with #1601 |
18c691c
to
1f4998f
Compare
@acoulton regarding your comment about the constructors, agree that your suggestion might be a better idea, updated. I also added a constant for the id of the configurable path printer service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks @carlos-granados - just one tiny thing I spotted in the diff.
1f4998f
to
a2ae00f
Compare
Currently we have several places in our code where we handle paths to relativize them. This PR replaces them with a single service that can print paths and which is configurable. And we add a new option which can be used to tell Behat to print all paths as absolute instead of relative paths. This option is available on the command line and also in the Php and Yaml configs